[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-    // These don't need to be particularly wide, because they're
-    // strictly limited by the forms of expressions we permit.
-    unsigned NumSubExprs : 8;
-    unsigned ResultIndex : 32 - 8 - NumExprBits;
+    unsigned NumSubExprs : 16;
+    unsigned ResultIndex : 16;
   };
----------------
aaron.ballman wrote:
> yronglin wrote:
> > dblaikie wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > dblaikie wrote:
> > > > > > Could/should we add some error checking in the ctor to assert that we don't overflow these longer values/just hit the bug later on?
> > > > > > 
> > > > > > (& could we use `unsigned short` here rather than bitfields?)
> > > > > We've already got them packed in with other bit-fields from the expression bits, so I think it's reasonable to continue the pattern of using bit-fields (that way we don't accidentally end up with padding between the unnamed bits at the start and the named bits in this object).
> > > > > 
> > > > > I think adding some assertions would not be a bad idea as a follow-up.
> > > > Maybe some unconditional (rather than only in asserts builds) error handling? (report_fatal_error, if this is low priority enough to not have an elegant failure mode, but something where we don't just overflow and carry on would be good... )
> > > Ping on this? I worry this code has just punted the same bug further down, but not plugged the hole/ensured we don't overflow on novel/larger inputs.
> > Sorry for the late reply, I was looking through the emails and found this. I agree add some assertions to check the value is a good idea, It's easy to help people catch bugs, at least with when `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, but one thing that worries me is that, in ASTReader, we access this field directly, not through the constructor or accessor, and we have to add assertions everywhere. https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> I don't think we have to add too many assertions. As best I can tell, we'll need one in each of the `PseudoObjectExpr` constructors and one in `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only places we assign a value into the bit-field. Three assertions isn't a lot, but if we're worried, we could add a setter method that does the assertion and use the setter in all three places.
My concern wasn't (well, wasn't entirely) about adding more assertions - but about having a reliable error here. The patch only makes the sizes larger, but doesn't have a hard-stop in case those sizes are exceeded again (which, admittedly, is much harder to do - maybe it's totally unreachable now, for all practical purposes?) 

I suspect with more carefully constructed recursive inputs could still reach the higher limit & I think it'd be good to fail hard in that case in some way? (it's probably rare enough that a report_fatal_error would be not-the-worst-thing-ever)

But good assertions would be nice too (the old code only failed when you hit /exactly/ on just the overflow value, and any more than that the wraparound would not crash/fail, but misbehave) - I did add the necessary assertion to ArrayRef (begin <= end) which would've helped detect this more reliably, but some assert checking for overflow in the ctor would be good too (with all the usual nuance/care in checking for overflow) - unless we're going to make that into a fatal or other real error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784