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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
