[Clang][Sema] Fix NULL dereferences for invalid references by DavidKorczynski · Pull Request #77703 · llvm/llvm-project (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

llvmbot added clang

Clang issues not falling into any other category

clang:frontend

Language frontend issues, e.g. anything involving "Sema"

labels

Jan 10, 2024

@llvm/pr-subscribers-clang

Author: None (DavidKorczynski)

Changes

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:

Full diff: https://github.com/llvm/llvm-project/pull/77703.diff

2 Files Affected:

(modified) clang/include/clang/Sema/ParsedAttr.h (+6-1)
(modified) clang/lib/Sema/SemaType.cpp (+3)

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 ||

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

erichkeane added a commit to erichkeane/llvm-project that referenced this pull request

Mar 1, 2024

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.

erichkeane added a commit that referenced this pull request

Mar 4, 2024

…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

Labels

clang:frontend

Language frontend issues, e.g. anything involving "Sema"

clang

Clang issues not falling into any other category