Page MenuHomePhabricator

Fix nullptr dereference found by Coverity static analysis tool
AcceptedPublic

Authored by schittir on Dec 1 2022, 2:50 PM.

Details

Reviewers
tahonermann
Group Reviewers
Restricted Project
Summary

Adding nullptr checks for 'Initializer'

Diff Detail

Event Timeline

schittir created this revision.Dec 1 2022, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 2:50 PM
schittir requested review of this revision.Dec 1 2022, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 2:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
schittir added a reviewer: Restricted Project.Dec 1 2022, 2:51 PM
shafik added a subscriber: shafik.Dec 1 2022, 5:16 PM

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?

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 updated this revision to Diff 479541.Dec 2 2022, 12:43 AM

Add more nullptr checks per Shafik's comments

tahonermann requested changes to this revision.Dec 2 2022, 8:39 AM
tahonermann added a subscriber: 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.

5933

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.

5942

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

5946

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

5978

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

6042–6043

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

6070–6075

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.

This revision now requires changes to proceed.Dec 2 2022, 8:39 AM
schittir added inline comments.Dec 2 2022, 12:02 PM
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?

5942

Indeed. This set of calls should be addressed too.

tahonermann added inline comments.Dec 2 2022, 12:22 PM
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 updated this revision to Diff 479731.Dec 2 2022, 1:28 PM

Add assert instead of multiple checks in if conditions.

schittir marked an inline comment as done and an inline comment as not done.Dec 2 2022, 1:28 PM

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

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.

I am bit afraid about release builds:

if (!Initializer)
  return;

Is this semantically incorrect?

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 updated this revision to Diff 480217.Dec 5 2022, 1:07 PM

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

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.

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.

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 requested changes to this revision.Dec 6 2022, 2:31 PM

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
This revision now requires changes to proceed.Dec 6 2022, 2:31 PM
clang/lib/Sema/SemaInit.cpp
5946

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

5966–5967

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

5978

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

6035

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

6042

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

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 updated this revision to Diff 481475.Dec 8 2022, 4:56 PM
schittir edited the summary of this revision. (Show Details)

Add assert at the beginning of blocks pointed out by Coverity

tahonermann added inline comments.Dec 9 2022, 12:30 PM
clang/lib/Sema/SemaInit.cpp
5959

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 updated this revision to Diff 481732.Dec 9 2022, 12:56 PM

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

tahonermann added inline comments.Dec 10 2022, 7:12 AM
clang/lib/Sema/SemaInit.cpp
5966–5967

Typo.

schittir updated this revision to Diff 481974.Dec 11 2022, 7:13 PM
schittir marked an inline comment as done.

Fixed typo. Sorry!

schittir updated this revision to Diff 481989.Dec 11 2022, 8:07 PM

Fix typos

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

tahonermann accepted this revision.Dec 12 2022, 10:23 AM

Builds passed now. I think this looks good.

This revision is now accepted and ready to land.Dec 12 2022, 10:23 AM