Skip to content

SemaObjC::HandleExprPropertyRefExpr assumes getInterfaceType() will never return nullptr #134954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shafik opened this issue Apr 9, 2025 · 3 comments · Fixed by #138026
Closed
Assignees
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" quality-of-implementation

Comments

@shafik
Copy link
Collaborator

shafik commented Apr 9, 2025

Static analysis has flagged, this line in SemaObjC::HandleExprPropertyRefExpr:

const ObjCInterfaceType *IFaceT = OPT->getInterfaceType();
ObjCInterfaceDecl *IFace = IFaceT->getDecl();

as possibly returning nullptr which would make the subsequent access of IFaceT->getDecl(); UB.

Based on the documentation:

/// HandleExprPropertyRefExpr - Handle foo.bar where foo is a pointer to an
/// objective C interface. This is a property reference expression.

and the rest of the code it seems clear that the assumption is that it will always be valid.

We could document more clearly w/ an assertion on IFaceT.

@shafik shafik self-assigned this Apr 9, 2025
@shafik
Copy link
Collaborator Author

shafik commented Apr 9, 2025

CC @rjmccall does this look like the right path?

@shafik shafik added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Apr 9, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 9, 2025

@llvm/issue-subscribers-clang-frontend

Author: Shafik Yaghmour (shafik)

Static analysis has flagged, this line in `SemaObjC::HandleExprPropertyRefExpr`:

const ObjCInterfaceType *IFaceT = OPT->getInterfaceType();
ObjCInterfaceDecl *IFace = IFaceT->getDecl();

as possibly returning nullptr which would make the subsequent access of IFaceT->getDecl(); UB.

Based on the documentation:

/// HandleExprPropertyRefExpr - Handle foo.bar where foo is a pointer to an
/// objective C interface. This is a property reference expression.

and the rest of the code it seems clear that the assumption is that it will always be valid.

We could document more clearly w/ an assertion on IFaceT.

@shafik
Copy link
Collaborator Author

shafik commented Apr 22, 2025

ping @rjmccall

shafik added a commit to shafik/llvm-project that referenced this issue Apr 30, 2025
Static analysis flagged that we use IFaceT in HandleExprPropertyRefExpr without
checking even though getInterfaceType() can return nullptr. The comments make it
clear the assumption is that we will always have an interface, so we will
document this via an assert.

Fixes: llvm#134954
@shafik shafik closed this as completed in 6543878 May 2, 2025
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
…vm#138026)

Static analysis flagged that we use IFaceT in HandleExprPropertyRefExpr
without checking even though getInterfaceType() can return nullptr. The
comments make it clear the assumption is that we will always have an
interface, so we will document this via an assert.

Fixes: llvm#134954
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
…vm#138026)

Static analysis flagged that we use IFaceT in HandleExprPropertyRefExpr
without checking even though getInterfaceType() can return nullptr. The
comments make it clear the assumption is that we will always have an
interface, so we will document this via an assert.

Fixes: llvm#134954
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
…vm#138026)

Static analysis flagged that we use IFaceT in HandleExprPropertyRefExpr
without checking even though getInterfaceType() can return nullptr. The
comments make it clear the assumption is that we will always have an
interface, so we will document this via an assert.

Fixes: llvm#134954
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this issue May 6, 2025
…RefExpr (#138026)

Static analysis flagged that we use IFaceT in HandleExprPropertyRefExpr
without checking even though getInterfaceType() can return nullptr. The
comments make it clear the assumption is that we will always have an
interface, so we will document this via an assert.

Fixes: llvm/llvm-project#134954
GeorgeARM pushed a commit to GeorgeARM/llvm-project that referenced this issue May 7, 2025
…vm#138026)

Static analysis flagged that we use IFaceT in HandleExprPropertyRefExpr
without checking even though getInterfaceType() can return nullptr. The
comments make it clear the assumption is that we will always have an
interface, so we will document this via an assert.

Fixes: llvm#134954
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" quality-of-implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants