[clang] [Clang] Fix potential null pointer dereferences in Sema::AddInitializerToDecl (PR #94368)

[Tom Honermann via cfe-commits]

tahonermann wrote:

I don't think the currently proposed change is right for a few reasons.

The proposed change silently marks the declaration as invalid and performs an early return. I don't think this should be a silent behavior. In most, if not all, of the existing early return cases, a diagnostic is issued (perhaps via another called function) and/or a recovery expression is created and used as the initializer.

I think this prior statement of mine is incorrect:
> On the one hand, the assignment to `Init` looks to me like it must produce a non-null result due to the prior check to `Result.isInvalid()`.

It is possible for an `ExprResult` value to be valid and still hold a null pointer value; `InitializationSequence::Perform()` explicitly returns a (valid) null pointer in at least one case:
```
clang/lib/Sema/SemaInit.cpp:
 8612 ExprResult InitializationSequence::Perform(Sema &S,
 8613                                            const InitializedEntity &Entity,
 8614                                            const InitializationKind &Kind,
 8615                                            MultiExprArg Args,
 8616                                            QualType *ResultType) {
 ....
 8699   // No steps means no initialization.
 8700   if (Steps.empty())
 8701     return ExprResult((Expr *)nullptr);
 ....
 9539 }
```

This indicates that, absent a more complicated analysis that proves that `Sema::AddInitializerToDecl()` will never call `InitializationSequence::Perform()` in a context that will lead to a null result, the static analysis tool is right to report a possible null dereference.

I suggest adding an assert here:
```
clang/lib/Sema/SemaDecl.cpp:
13660   if (!VDecl->isInvalidDecl()) {
.....
13690     ExprResult Result = InitSeq.Perform(*this, Entity, Kind, Args, &DclT);
13691     if (Result.isInvalid()) {
.....
13707       return;
13708     }
13709
13710     Init = Result.getAs<Expr>();
.....
13726   }
    + 
    +   assert(Init && "Should have a valid initializer at this point);
    + 
13728   // Check for self-references within variable initializers.
13729   // Variables declared within a function/method body (except for references)
13730   // are handled by a dataflow analysis.
13731   // This is undefined behavior in C++, but valid in C.
13732   if (getLangOpts().CPlusPlus)
13733     if (!VDecl->hasLocalStorage() || VDecl->getType()->isRecordType() ||
13734         VDecl->getType()->isReferenceType())
13735       CheckSelfReference(*this, RealDecl, Init, DirectInit);
```

Relatedly, I think the two checks for `Init` being non-null are probably wrong in these cases:
```
13506   // WebAssembly tables can't be used to initialise a variable.
13507   if (Init && !Init->getType().isNull() &&
13508       Init->getType()->isWebAssemblyTableType()) {
13509     Diag(Init->getExprLoc(), diag::err_wasm_table_art) << 0;
13510     VDecl->setInvalidDecl();
13511     return;
13512   }
.....
13715     if (Init && !Init->getType().isNull() &&
13716         !Init->getType()->isDependentType() && !VDeclType->isDependentType() &&
13717         Context.getAsIncompleteArrayType(VDeclType) &&
13718         Context.getAsIncompleteArrayType(Init->getType())) {
13719       // Bail out if it is not possible to deduce array size from the
13720       // initializer.
13721       Diag(VDecl->getLocation(), diag::err_typecheck_decl_incomplete_type)
13722           << VDeclType;
13723       VDecl->setInvalidDecl();
13724       return;
13725     }
```

I think the null check at line 13507 can just be removed; previous uses of `Init` appear to expect a non-null value. This looks like a case of a reflexive defensive check (added in https://reviews.llvm.org/D139010).

The null check at line 13715 also looks like it was added as a reflexive defensive check (added in https://reviews.llvm.org/D158615 for https://github.com/llvm/llvm-project/issues/37257)

Adding the assert and removing the defensive null checks would eliminate the evidence the static analysis tool is using to justify its report of a potential null dereference.

If Clang tests pass with these changes (using an asserts enabled build of course!), then I think we would be ok to proceed. If we then see failed assertions or null pointer dereferences at some point, we'll be able to identify test cases to add.

https://github.com/llvm/llvm-project/pull/94368