[Clang][Sema] Fix NULL dereferences for invalid references #77703
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (DavidKorczynski) ChangesOSS-Fuzz has reported for a bit of time (since early 2020) a couple of NULL dereferences due to the Info reference becoming a reference to a NULL pointer. Am not entirely sure if this is the desired fix since NULL checking on reference may not be considered a great practice, but am submitting for review in case it's acceptable. Fixes:
Full diff: https://github.com/llvm/llvm-project/pull/77703.diff 2 Files Affected:
diff --git a/clang/include/clang/Sema/ParsedAttr.h b/clang/include/clang/Sema/ParsedAttr.h
index 8c0edca1ebc5ee..70877f8c45cec2 100644
--- a/clang/include/clang/Sema/ParsedAttr.h
+++ b/clang/include/clang/Sema/ParsedAttr.h
@@ -342,7 +342,12 @@ class ParsedAttr final
return IsProperty;
}
- bool isInvalid() const { return Invalid; }
+ bool isInvalid() const {
+ if (&Info == NULL) {
+ Invalid = true;
+ }
+ return Invalid;
+ }
void setInvalid(bool b = true) const { Invalid = b; }
bool hasProcessingCache() const { return HasProcessingCache; }
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index a376f20fa4f4e0..40abb3a197faa5 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -4240,6 +4240,9 @@ IdentifierInfo *Sema::getNSErrorIdent() {
/// attribute list.
static bool hasNullabilityAttr(const ParsedAttributesView &attrs) {
for (const ParsedAttr &AL : attrs) {
+ if (AL.isInvalid()) {
+ continue;
+ }
if (AL.getKind() == ParsedAttr::AT_TypeNonNull ||
AL.getKind() == ParsedAttr::AT_TypeNullable ||
AL.getKind() == ParsedAttr::AT_TypeNullableResult ||
|
OSS-Fuzz has reported for a bit of time (since early 2020) a couple of NULL dereferences due to the Info reference becoming a reference to a NULL pointer. Am not entirely sure if this is the desired fix since NULL checking on reference may not be considered a great practice, but am submitting for review in case it's acceptable. Fixes: - https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20946 - https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20938 Signed-off-by: David Korczynski <[email protected]>
There was a problem hiding this comment.
I don't believe this is the right approach.
I can only replicate one of the issues: https://godbolt.org/z/7dee3a3cY
I spent some time looking at it and it is quite gnarly but we need to understand better what is going on.
|
Yeah, this doesn't seem right to me. Info is a reference, and thus cannot be null by language rule. A sufficiently smart compiler will remove your check. If we're SOMEHOW (though I don't see how?) setting it to nullptr, we need to fix that as that is not intended, nor particularly easy from what I can tell. |
|
I spent a while debugging this, and this is far from the correct solution. However, in my debugging I was able to identify the correct solution, so I'll prepare a patch to fix this, closing. |
This was reported (sort of) in a PR: llvm#77703. The problem is that a declarator 'owns' an attributes allocation via an `AttributePool`. However, this example tries to copy a DeclaratorChunk from one Declarator to another, so when the temporary Declarator goes out of scope, it deletes the attribute it has tried to pass on via the chunk. This patch ensures that we copy the 'ownership' of the attribute correctly, and adds an assert to catch any other casess where this happens.
…83611) This was reported (sort of) in a PR: #77703. The problem is that a declarator 'owns' an attributes allocation via an `AttributePool`. However, this example tries to copy a DeclaratorChunk from one Declarator to another, so when the temporary Declarator goes out of scope, it deletes the attribute it has tried to pass on via the chunk. This patch ensures that we copy the 'ownership' of the attribute correctly, and adds an assert to catch any other casess where this happens. Additionally, this was put in as a bug report, so this Fixes #83611
OSS-Fuzz has reported for a bit of time (since early 2020) a couple of NULL dereferences due to the Info reference becoming a reference to a NULL pointer.
Am not entirely sure if this is the desired fix since NULL checking on reference may not be considered a great practice, but am submitting for review in case it's acceptable.
Fixes: