[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool
Tom Honermann via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 5 07:32:21 PST 2022
tahonermann added a comment.
> I am bit afraid about release builds:
>
> if (!Initializer)
>
> return;
>
> Is this semantically incorrect?
I think so. And now I think I see a path where the proposed `assert` is incorrect as well. The `else` branches at lines 5919, 5921, and 5923 appear to handle cases where `Initializer` may be null.
clang/lib/Sema/SemaInit.cpp:
5824 // Handle default initialization.
5825 if (Kind.getKind() == InitializationKind::IK_Default) {
5826 TryDefaultInitialization(S, Entity, Kind, *this);
5827 return;
5828 }
<Proposed assertion>
....
5835 if (const ArrayType *DestAT = Context.getAsArrayType(DestType)) {
5836 if (Initializer && isa<VariableArrayType>(DestAT)) {
....
5912 else if (S.getLangOpts().CPlusPlus &&
5913 Entity.getKind() == InitializedEntity::EK_Member &&
5914 Initializer && isa<InitListExpr>(Initializer)) {
....
5918 } else if (DestAT->getElementType()->isCharType())
5919 SetFailed(FK_ArrayNeedsInitListOrStringLiteral);
5920 else if (IsWideCharCompatible(DestAT->getElementType(), Context))
5921 SetFailed(FK_ArrayNeedsInitListOrWideStringLiteral);
5922 else
5923 SetFailed(FK_ArrayNeedsInitList);
5924
5925 return;
5926 }
Perhaps the` assert` should be added after line 5926 above. I would be concerned about adding a return statement conditional on `Initializer` being null there though. If the intention is that an initializer must be present after that point, an early return could result in a miscompile; I'd rather have a crash.
I think the checks for `Initializer` being non-null following the addition of an `assert` should be removed.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139148/new/
https://reviews.llvm.org/D139148
More information about the cfe-commits
mailing list