[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool
Tom Honermann via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 2 08:39:12 PST 2022
tahonermann requested changes to this revision.
tahonermann added a comment.
This revision now requires changes to proceed.
Per added comments, I think we should look for a guarantee that `Initializer` is non-null earlier in the function. If there is, then we could remove a bunch of the current existence checks rather than adding more.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5824-5828
// Handle default initialization.
if (Kind.getKind() == InitializationKind::IK_Default) {
TryDefaultInitialization(S, Entity, Kind, *this);
return;
}
----------------
This block handles default initialization and unconditionally performs a return. I wonder if this effectively guarantees that `Initializer` is non-null if this block is not entered.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5933
if (TryOCLSamplerInitialization(S, *this, DestType, Initializer))
return;
----------------
The use of `initializer` here looks questionable too; `TryOCLSamplerInitialization()` will dereference it without a check if both `S.getLangOpts().OpenCL` and `DestType->isSamplerT()` are both true.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5941
if (allowObjCWritebackConversion &&
tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
return;
----------------
This use of `Initializer` is also questionable; `tryObjCWritebackConversion()` will unconditionally dereference it.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5945
if (TryOCLZeroOpaqueTypeInitialization(S, *this, DestType, Initializer))
return;
----------------
This use of `Initializer` is also questionable; `TryOCLZeroOpaqueTypeInitialization()` will conditionally dereference it.
================
Comment at: clang/lib/Sema/SemaInit.cpp:5976
else
TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
----------------
This use of `Initializer` looks like it also needs to be protected; `TryUserDefinedConversion()` unconditionally dereferences it.
================
Comment at: clang/lib/Sema/SemaInit.cpp:6038-6039
TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
TopLevelOfInitList);
MaybeProduceObjCObject(S, *this, Entity);
----------------
This use of `Initializer` looks like it also needs to be protected; `TryUserDefinedConversion()` unconditionally dereferences it.
================
Comment at: clang/lib/Sema/SemaInit.cpp:6066-6071
= S.TryImplicitConversion(Initializer, DestType,
/*SuppressUserConversions*/true,
Sema::AllowedExplicit::None,
/*InOverloadResolution*/ false,
/*CStyle=*/Kind.isCStyleOrFunctionalCast(),
allowObjCWritebackConversion);
----------------
`Initializer` is unconditionally dereferenced in `Sema::TryImplicitConversion()`.
I stopped analyzing other uses here. At this point (at least), it seems clear that the expectation is that `Initializer` is non-null. That makes me think that, rather than adding additional checks, we should look for an existing guarantee that `initializer` is in fact non-null (something that Coverity missed) or add one. If we need to add such a guarantee, we could add an `assert(Initializer)` somewhere earlier in the function, but I'm not sure where.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139148/new/
https://reviews.llvm.org/D139148
More information about the cfe-commits
mailing list