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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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;
   };
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154784