[PATCH] D136671: [analyzer] Fix crash for array-delete of UnknownVal values.

[Tomasz Kamiński via Phabricator via cfe-commits]

tomasz-kaminski-sonarsource added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1244
                                             SVal *ElementCountVal) {
+  assert(Region != nullptr && "Null-regions passed to prepareStateForArrayDestruction.");
 
----------------
isuckatcs wrote:
> I meant to assert this inside `getDynamicElementCount()` as that's the function that dereferences the pointer.
> 
> ```
> DefinedOrUnknownSVal getDynamicElementCount(..., const MemRegion *MR, ...) {
>   MR = MR->StripCasts();                                                            <-- crash here if MR is a nullptr
> 
>   DefinedOrUnknownSVal Size = getDynamicExtent(State, MR, SVB);
>   SVal ElementSize = getElementExtent(ElementTy, SVB);
> 
>   SVal ElementCount =
>       SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType());     <-- this could be the origin of 
>                                                                                         a crash too
> 
>   return ElementCount.castAs<DefinedOrUnknownSVal>();
> }
> ```
> 
> Also `SVB.evalBinOp()` can technically return an `UndefinedVal` and if it does that, then `ElementCount.castAs<DefinedOrUnknownSVal>()` will also cause a crash as it happened in D130974.
> 
> Also since this function can return an `UnknownSVal`, what do you think about returning that if `MR` is a `nullptr` instead of stopping execution? Would that behaviour even make sense?
> I meant to assert this inside `getDynamicElementCount()` as that's the function that dereferences the pointer.
> 
> ```
> DefinedOrUnknownSVal getDynamicElementCount(..., const MemRegion *MR, ...) {
>   MR = MR->StripCasts();                                                            <-- crash here if MR is a nullptr
> 
>   DefinedOrUnknownSVal Size = getDynamicExtent(State, MR, SVB);
>   SVal ElementSize = getElementExtent(ElementTy, SVB);
> 
>   SVal ElementCount =
>       SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType());     <-- this could be the origin of 
>                                                                                         a crash too
> 
>   return ElementCount.castAs<DefinedOrUnknownSVal>();
> }
> ```
> 
> Also `SVB.evalBinOp()` can technically return an `UndefinedVal` and if it does that, then `ElementCount.castAs<DefinedOrUnknownSVal>()` will also cause a crash as it happened in D130974.
> 
> Also since this function can return an `UnknownSVal`, what do you think about returning that if `MR` is a `nullptr` instead of stopping execution? Would that behaviour even make sense?




================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1244
                                             SVal *ElementCountVal) {
+  assert(Region != nullptr && "Null-regions passed to prepareStateForArrayDestruction.");
 
----------------
tomasz-kaminski-sonarsource wrote:
> isuckatcs wrote:
> > I meant to assert this inside `getDynamicElementCount()` as that's the function that dereferences the pointer.
> > 
> > ```
> > DefinedOrUnknownSVal getDynamicElementCount(..., const MemRegion *MR, ...) {
> >   MR = MR->StripCasts();                                                            <-- crash here if MR is a nullptr
> > 
> >   DefinedOrUnknownSVal Size = getDynamicExtent(State, MR, SVB);
> >   SVal ElementSize = getElementExtent(ElementTy, SVB);
> > 
> >   SVal ElementCount =
> >       SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType());     <-- this could be the origin of 
> >                                                                                         a crash too
> > 
> >   return ElementCount.castAs<DefinedOrUnknownSVal>();
> > }
> > ```
> > 
> > Also `SVB.evalBinOp()` can technically return an `UndefinedVal` and if it does that, then `ElementCount.castAs<DefinedOrUnknownSVal>()` will also cause a crash as it happened in D130974.
> > 
> > Also since this function can return an `UnknownSVal`, what do you think about returning that if `MR` is a `nullptr` instead of stopping execution? Would that behaviour even make sense?
> > I meant to assert this inside `getDynamicElementCount()` as that's the function that dereferences the pointer.
> > 
> > ```
> > DefinedOrUnknownSVal getDynamicElementCount(..., const MemRegion *MR, ...) {
> >   MR = MR->StripCasts();                                                            <-- crash here if MR is a nullptr
> > 
> >   DefinedOrUnknownSVal Size = getDynamicExtent(State, MR, SVB);
> >   SVal ElementSize = getElementExtent(ElementTy, SVB);
> > 
> >   SVal ElementCount =
> >       SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType());     <-- this could be the origin of 
> >                                                                                         a crash too
> > 
> >   return ElementCount.castAs<DefinedOrUnknownSVal>();
> > }
> > ```
> > 
> > Also `SVB.evalBinOp()` can technically return an `UndefinedVal` and if it does that, then `ElementCount.castAs<DefinedOrUnknownSVal>()` will also cause a crash as it happened in D130974.
> > 
> > Also since this function can return an `UnknownSVal`, what do you think about returning that if `MR` is a `nullptr` instead of stopping execution? Would that behaviour even make sense?
> 
> 
> I meant to assert this inside `getDynamicElementCount()` as that's the function that dereferences the pointer.
I see. I will add the assert.

> Also since this function can return an `UnknownSVal`, what do you think about returning that if `MR` is a `nullptr` instead of stopping execution? Would that behaviour even make sense?

I still believe that passing `nullptr` as `MR` to this function is bug on the caller side, and not expected behavior. This is why I would prefer to have to assert here, instead of returning some symbol, as this will hide such bugs.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136671/new/

https://reviews.llvm.org/D136671