[PATCH] D139148: Fix nullptr dereference found by Coverity static analysis tool

[Tom Honermann via Phabricator via cfe-commits]

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