Fix nullptr dereference found by Coverity static analysis tool

[schittir]

Summary

Adding nullptr checks for 'Initializer'

[shafik]

This looks like the correct thing to do but I see further down there are also unconditional uses of Initializer that should also be guarded with a check. It would be good to fix those as well since we are here.

I am guessing the answer is no since this was found using a static analysis tool but do you have a test case that triggers this failure?

[schittir]

In D139148#3965404, @shafik wrote:

This looks like the correct thing to do but I see further down there are also unconditional uses of Initializer that should also be guarded with a check. It would be good to fix those as well since we are here.

Yes, it does make sense.

I am guessing the answer is no since this was found using a static analysis tool but do you have a test case that triggers this failure?

Indeed, the answer is no.

[schittir]

Add more nullptr checks per Shafik's comments

[tahonermann]

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.

clang/lib/Sema/SemaInit.cpp

5824–5828

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.

5935

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.

5943

This use of Initializer is also questionable; tryObjCWritebackConversion() will unconditionally dereference it.

5947

This use of Initializer is also questionable; TryOCLZeroOpaqueTypeInitialization() will conditionally dereference it.

5977

This use of Initializer looks like it also needs to be protected; TryUserDefinedConversion() unconditionally dereferences it.

6039–6040

This use of Initializer looks like it also needs to be protected; TryUserDefinedConversion() unconditionally dereferences it.

6067–6072

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.

[schittir]

clang/lib/Sema/SemaInit.cpp

5824–5828

Thank you for the review. I am trying to find an explicit connection between this block and Initializer being non-null, but it isn't clear to me yet.
How about adding the assert right after this block?

5943

Indeed. This set of calls should be addressed too.

[tahonermann]

clang/lib/Sema/SemaInit.cpp

5824–5828

I think adding an assert here is a good thing to try. If any tests fail, then we'll know this isn't the right place. If no tests fail, then, well, the assert is good or we need more tests! ;)

[schittir]

Add assert instead of multiple checks in if conditions.

[tahonermann]

The change to add the assert looks good. I'm eager to see what the testsuite has to say about it :)

[tahonermann]

When committing changes to address Coverity reported issues, I think it would be useful to include the Coverity analysis in the commit message. A textual representation can be obtained using cov-format-errors --emacs-style from the command line with access to the intermediate directory. I'm not sure if a textual representation can be obtained through the Coverity UI though.

[tschuett]

I am bit afraid about release builds:

if (!Initializer)
  return;

Is this semantically incorrect?

[tahonermann]

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.

[schittir]

Move the assert to after the branches that handle the cases where Initializer may be null

[schittir]

Perhaps the` assert` should be added after line 5926 above.

Done.

I think the checks for Initializer being non-null following the addition of an assert should be removed.

I don't see any after this point.

[schittir]

In D139148#3969383, @tahonermann wrote:

When committing changes to address Coverity reported issues, I think it would be useful to include the Coverity analysis in the commit message. A textual representation can be obtained using cov-format-errors --emacs-style from the command line with access to the intermediate directory. I'm not sure if a textual representation can be obtained through the Coverity UI though.

Couldn't find a way to do this via the UI, so I am trying the command line way.

[tahonermann]

I don't see any after this point.

You're right! I thought I had seen some, but upon checking, I agree. That is a good sign we're on the right path!

[tahonermann]

Hmm, it seems the assert doesn't work in that location. The Linux and Windows builds both failed. It would be good to identify a simple test case for that. It seems we'll need to move the assert further down the function body.

FAILED: tools/clang/lib/Tooling/ASTNodeAPI.json
cmd.exe /C "cd /D C:\ws\w8\llvm-project\premerge-checks\build\tools\clang\lib\Tooling && C:\ws\w8\llvm-project\premerge-checks\build\bin\clang-ast-dump.exe --skip-processing=0 -I C:/ws/w8/llvm-project/premerge-checks/build/lib/clang/16/include -I C:/ws/w8/llvm-project/premerge-checks/clang/include -I C:/ws/w8/llvm-project/premerge-checks/build/tools/clang/include -I C:/ws/w8/llvm-project/premerge-checks/build/include -I C:/ws/w8/llvm-project/premerge-checks/llvm/include --json-output-path C:/ws/w8/llvm-project/premerge-checks/build/tools/clang/lib/Tooling/ASTNodeAPI.json"
Assertion failed: Initializer && "Intializer must be non-null", file C:\ws\w8\llvm-project\premerge-checks\clang\lib\Sema\SemaInit.cpp, line 5928

[schittir]

clang/lib/Sema/SemaInit.cpp

5947

#1/5 Coverity reported explicit null dereference of 'Initializer'

5967

#2/5 Coverity reported explicit null dereference of 'Initializer'

5977

#3/5 Coverity reported explicit null dereference of 'Initializer'

6032

#4/5 Coverity reported explicit null dereference of 'Initializer'

6039

#5/5 Coverity reported explicit null dereference of 'Initializer'

[tahonermann]

Here is a simple test case that fails the assertion in the current location.

struct B {
  B(int, int);
};
struct D : B {
  D() : B(0, 1) {}
};

I spent a bit more time looking at the code and finally realized that Initializer is only assigned when Args.size() is exactly 1. So Initializer doesn't equate to whether or not an initializer is present; it equates to when exactly one initialization argument is present and in that case it aliases Args[0]. I don't like that, but deviating from it just to address a Coverity complaint doesn't seem justified.

I suggest we do this: add an assertion on Initializer at the beginning of each of the blocks that Coverity complained about. In my view of the source, that corresponds to lines 5939, 5958, and 6025.

[schittir]

Add assert at the beginning of blocks pointed out by Coverity

[tahonermann]

clang/lib/Sema/SemaInit.cpp

5960

It looks like this assertion was triggered for both the Linux and Windows builds.

Since the else branch below unconditionally dereferences Initializer, I think the only way for Initializer to be null and for a crash not to occur is if the then branch is taken, but without Initializer->getBeginLoc() being evaluated due to short circuiting. I think we should do this:

1. Add a Initializer && condition prior to the call to S.IsDerivedFrom(Initializer->getBeginLoc(), ...). This will require yet more parenthesis for the conditional logic.
2. Add the assert to the else branch.

[schittir]

Add (Initializer && and assert to else branch per Tom's comments.

[tahonermann]

clang/lib/Sema/SemaInit.cpp

5967

Typo.

[schittir]

Fixed typo. Sorry!

[schittir]

Fix typos

[tahonermann]

The build failed again, but it looks like the cause is unrelated to these changes. I restarted the build.

[tahonermann]

Builds passed now. I think this looks good.

[schittir]

Landing this one fell through the cracks, hence the delay between accepting and committing the patch.

clang/lib/Sema/SemaInit.cpp

void InitializationSequence::InitializeFrom(Sema &S,
 
   //     - If the initializer is (), the object is value-initialized.
   if (Kind.getKind() == InitializationKind::IK_Value ||
       (Kind.getKind() == InitializationKind::IK_Direct && Args.empty())) {
     TryValueInitialization(S, Entity, Kind, *this);
     return;
   }
 
   // Handle default initialization.
   if (Kind.getKind() == InitializationKind::IK_Default) {
     TryDefaultInitialization(S, Entity, Kind, *this);
     return;
   }
 
   //     - If the destination type is an array of characters, an array of
   //       char16_t, an array of char32_t, or an array of wchar_t, and the
   //       initializer is a string literal, see 8.5.2.
   //     - Otherwise, if the destination type is an array, the program is
   //       ill-formed.
   if (const ArrayType *DestAT = Context.getAsArrayType(DestType)) {
     if (Initializer && isa<VariableArrayType>(DestAT)) {
if (const ArrayType *DestAT = Context.getAsArrayType(DestType)) {
     else if (IsWideCharCompatible(DestAT->getElementType(), Context))
       SetFailed(FK_ArrayNeedsInitListOrWideStringLiteral);
     else
       SetFailed(FK_ArrayNeedsInitList);
 
     return;
   }
 
+  assert(Initializer && "Intializer must be non-null");
+
   // Determine whether we should consider writeback conversions for
   // Objective-C ARC.
   bool allowObjCWritebackConversion = S.getLangOpts().ObjCAutoRefCount &&
          Entity.isParameterKind();
 
   if (TryOCLSamplerInitialization(S, *this, DestType, Initializer))
     return;
 
   // We're at the end of the line for C: it's either a write-back conversion
   // or it's a C assignment. There's no need to check anything else.
   if (!S.getLangOpts().CPlusPlus) {
     // If allowed, check whether this is an Objective-C writeback conversion.
     if (allowObjCWritebackConversion &&
         tryObjCWritebackConversion(S, *this, Entity, Initializer)) {
       return;
     }
 
     if (TryOCLZeroOpaqueTypeInitialization(S, *this, DestType, Initializer))
       return;
 
     // Handle initialization in C
     AddCAssignmentStep(DestType);
     MaybeProduceObjCObject(S, *this, Entity);
     return;
   }
 
   assert(S.getLangOpts().CPlusPlus);
 
   //     - If the destination type is a (possibly cv-qualified) class type:
   if (DestType->isRecordType()) {
     //     - If the initialization is direct-initialization, or if it is
     //       copy-initialization where the cv-unqualified version of the
     //       source type is the same class as, or a derived class of, the
     //       class of the destination, constructors are considered. [...]
     if (Kind.getKind() == InitializationKind::IK_Direct ||
         (Kind.getKind() == InitializationKind::IK_Copy &&
          (Context.hasSameUnqualifiedType(SourceType, DestType) ||
           S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType, DestType))))
       TryConstructorInitialization(S, Entity, Kind, Args,
                                    DestType, DestType, *this);
     //     - Otherwise (i.e., for the remaining copy-initialization cases),
     //       user-defined conversion sequences that can convert from the source
     //       type to the destination type or (when a conversion function is
     //       used) to a derived class thereof are enumerated as described in
     //       13.3.1.4, and the best one is chosen through overload resolution
     //       (13.3).
     else
       TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
                                TopLevelOfInitList);
     return;
   }
 
   assert(Args.size() >= 1 && "Zero-argument case handled above");
 
   // For HLSL ext vector types we allow list initialization behavior for C++
   // constructor syntax. This is accomplished by converting initialization
void InitializationSequence::InitializeFrom(Sema &S,
   //    - Otherwise, if the source type is a (possibly cv-qualified) class
   //      type, conversion functions are considered.
   if (!SourceType.isNull() && SourceType->isRecordType()) {
     // For a conversion to _Atomic(T) from either T or a class type derived
     // from T, initialize the T object then convert to _Atomic type.
     bool NeedAtomicConversion = false;
     if (const AtomicType *Atomic = DestType->getAs<AtomicType>()) {
       if (Context.hasSameUnqualifiedType(SourceType, Atomic->getValueType()) ||
           S.IsDerivedFrom(Initializer->getBeginLoc(), SourceType,
                           Atomic->getValueType())) {
         DestType = Atomic->getValueType();
         NeedAtomicConversion = true;
       }
     }
 
     TryUserDefinedConversion(S, DestType, Kind, Initializer, *this,
                              TopLevelOfInitList);
     MaybeProduceObjCObject(S, *this, Entity);
     if (!Failed() && NeedAtomicConversion)
       AddAtomicConversionStep(Entity.getType());
     return;
   }
 
   //    - Otherwise, if the initialization is direct-initialization, the source
   //    type is std::nullptr_t, and the destination type is bool, the initial
void InitializationSequence::InitializeFrom(Sema &S,
 
   //    - Otherwise, the initial value of the object being initialized is the
   //      (possibly converted) value of the initializer expression. Standard
   //      conversions (Clause 4) will be used, if necessary, to convert the
   //      initializer expression to the cv-unqualified version of the
   //      destination type; no user-defined conversions are considered.
 
   ImplicitConversionSequence ICS
     = S.TryImplicitConversion(Initializer, DestType,
                               /*SuppressUserConversions*/true,
                               Sema::AllowedExplicit::None,
                               /*InOverloadResolution*/ false,
                               /*CStyle=*/Kind.isCStyleOrFunctionalCast(),
                               allowObjCWritebackConversion);
 
   if (ICS.isStandard() &&
       ICS.Standard.Second == ICK_Writeback_Conversion) {
     // Objective-C ARC writeback conversion.
 
     // We should copy unless we're passing to an argument explicitly
     // marked 'out'.
     bool ShouldCopy = true;