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

[yronglin]

Summary

This patch makes the bit-fields wider, and also implement a small optimization for PseudoObjectExprBitfields, when there is no result in PseudoObjectExpr, we use 32 bits to store the number of subexpressions, otherwise, we use 16 bits to store the number of subexpressions, and use 16 bits to store the result indexes.

Fixes https://github.com/llvm/llvm-project/issues/63169

[yronglin]

Format

[yronglin]

I'm not sure if we should limit the value of NumSubExprs when build PseudoObjectExpr for __builtin_dump_struct, This cost too much memory when the nested members of the record are very deep and the num of member is very large.

[aaron.ballman]

In general, I think this is a good approach. However, it sort of kicks the can down the road a bit; we will still overflow the member if there are enough fields. Would it make sense to also add a diagnostic to Sema so that overflow with the widened fields is diagnosed rather than causing a crash?

clang/include/clang/AST/Stmt.h

596–597

clang/test/SemaCXX/builtin-dump-struct.cpp

163

[yronglin]

Address Aaron's comments.

[yronglin]

Update comments in PseudoObjectExpr.

[yronglin]

In D154784#4485752, @aaron.ballman wrote:

In general, I think this is a good approach. However, it sort of kicks the can down the road a bit; we will still overflow the member if there are enough fields. Would it make sense to also add a diagnostic to Sema so that overflow with the widened fields is diagnosed rather than causing a crash?

Thanks you for take a look!

Would it make sense to also add a diagnostic to Sema so that overflow with the widened fields is diagnosed rather than causing a crash?

Ah, I think your are right. It doesn't make sense, As the comments for PseudoObjectExprBitfields says These don't need to be particularly wide, because they're strictly limited by the forms of expressions we permit. I think developers who use PseudoObjectExprBitfields need to be more careful.

[rjmccall]

We had that discussion in the bug report. Let's just increase the widths unconditionally; this is not such a common expression class that we need to worry about using an extra word.

[yronglin]

In D154784#4486721, @rjmccall wrote:

We had that discussion in the bug report. Let's just increase the widths unconditionally; this is not such a common expression class that we need to worry about using an extra word.

Thanks for your review!

Let's just increase the widths unconditionally

Do you mean:

class PseudoObjectExprBitfields {
    friend class ASTStmtReader; // deserialization
    friend class PseudoObjectExpr;

    unsigned : NumExprBits;
    unsigned NumSubExprs : 16;
    unsigned ResultIndex : 16;
  };

There are 14 padding bits we can not use it to extent NumSubExprs and ResultIndex which after unsigned : NumExprBits

[yronglin]

Address John's comment.

[rjmccall]

clang/include/clang/AST/Stmt.h

596–597

Please remove the comment, which is incorrect. Otherwise, I think this is fine.

[aaron.ballman]

LGTM, thank you for the fix!

[yronglin]

Address comment

[yronglin]

clang/include/clang/AST/Stmt.h

596–597

Thanks, removed.

[yronglin]

Thanks for your review! @aaron.ballman @rjmccall

[yronglin]

Rebase

[yronglin]

Wait for CI green

[dblaikie]

clang/include/clang/AST/Stmt.h

596–597

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?)

[aaron.ballman]

clang/include/clang/AST/Stmt.h

596–597

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.

[dblaikie]

clang/include/clang/AST/Stmt.h

596–597

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... )

[dblaikie]

clang/include/clang/AST/Stmt.h

596–597

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.

[yronglin]

clang/include/clang/AST/Stmt.h

596–597

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

[aaron.ballman]

clang/include/clang/AST/Stmt.h

596–597

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.

[dblaikie]

clang/include/clang/AST/Stmt.h

596–597

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.

[yronglin]

clang/include/clang/AST/Stmt.h

596–597

Sorry for the very late reply. I have no preference between assertion and llvm_unreachable, if error then fail fast is looks good. I have a patch D158296 to add assertion.

[dblaikie]

clang/include/clang/AST/Stmt.h

596–597

Thanks for the assertions - though they still haven't met my main concern that this should have a hard failure even in a non-assertions build.

I know we don't have a perfect plan/policy for these sort of "run out of resources/hit a representational limit" issues (at least I don't think we do... do we, @aaron.ballman ? I know we have some limits (recursion, template expansion, etc) but they're fairly specific/aren't about every possible case of integer overflow in some representational element, etc) but we've seen this one is pretty reachable.

Here's a test case that would still trigger the assertion, and trigger UB in a non-assertions build:

truct t1 { };
template<typename T1>
struct templ {
    T1 v1;
    T1 v2;
    T1 v3;
    T1 v4;
};

struct t2 {
  templ<templ<templ<templ<templ<templ<t1>>>>>> c0;
  templ<templ<templ<templ<templ<templ<t1>>>>>> c1;
  templ<templ<templ<templ<templ<templ<t1>>>>>> c2;
};

void aj(...);
void f1(t2 w) { __builtin_dump_struct(&w, aj); }

(used templates to pack this a bit more densely than the original test case) - the sizeof the struct is certainly a bit outlandish (~12kbytes) bit not, I think, totally unreasonable?

[yronglin]

clang/include/clang/AST/Stmt.h

596–597

Thanks for your example. I have three ways:

1. use llvm_unreachable to emit a hard failure but not an assertion.
2. extend these two field to 32-bit unsigned, it's may big enough.
3. limit the functionality of __builtin_dump_struct, if there are too many fields in a struct, the part exceeding the limit will not be output, and replaced with ...(maybe).

WDYT? You guys are expert in clang, and I would like to wait for your guidance :)

[yronglin]

clang/include/clang/AST/Stmt.h

596–597

We have ever limit the length of c-string value in __builtin_dump_struct. https://github.com/llvm/llvm-project/blob/5675f44cebf602931e884595c1d488bcedf6b8f2/clang/lib/Sema/SemaChecking.cpp#L524-L527

[aaron.ballman]

clang/include/clang/AST/Stmt.h

596–597

I know we don't have a perfect plan/policy for these sort of "run out of resources/hit a representational limit" issues (at least I don't think we do... do we, @aaron.ballman ? I know we have some limits (recursion, template expansion, etc) but they're fairly specific/aren't about every possible case of integer overflow in some representational element, etc) but we've seen this one is pretty reachable.

Correct, we don't have a general mechanism for handling resource limits, we mostly play whack-a-mole when we run into them. So having a general utility that can work for other bit-fields would be really nice. However, __builtin_dump_struct is a debugging interface and not something we expect users to call particularly often, so some sharp edges with the interface are not the end of the world IMO.

limit the functionality of __builtin_dump_struct, if there are too many fields in a struct, the part exceeding the limit will not be output, and replaced with ...(maybe).

I think this is basically what @dblaikie was asking for -- if there are too many fields in the structure, either give a diagnostic that the structure is too complex for us to dump, or cut off the output somewhere sensible, etc. @dblaikie, do you have a preference for diagnostic vs prettier output? Ideally, I lean towards prettier output, but at the same time, that might be a bigger ask than what you were after.

[dblaikie]

clang/include/clang/AST/Stmt.h

596–597

Yeah, I don't have major/heroic requirements here - just that we don't crash on these inputs. An error, wider integers, both, etc, it's all good to me.

just an error seems fine to me - as you say, it's basically a debugging interface, people shouldn't be making this some loadbearing/totally generic part of an API or somesuch.

[aaron.ballman]

clang/include/clang/AST/Stmt.h

596–597

Thank you! I'm also fine with going with just an error.

[yronglin]

clang/include/clang/AST/Stmt.h

596–597

I'm fine too! But I have some concerns, the BuiltinDumpStructGenerator create call of print functions. but the call of print functions not only for field, but also for { and '}', so I have no idea to describe the problem in the diagnostic message, 'too many fields in struct' doesn't feel right to me. Do you have any other good idea? I'd be happy to continue cook D158296

[aaron.ballman]

clang/include/clang/AST/Stmt.h

596–597

Hmm, I think I'd be fine with something along the lines of: class|struct|union 'blah' is too complex to dump and leave "too complex" vague (because this is a debugging interface and it involves some quite questionably-sized objects, I think it's fine for the QoI bar to be lower for this diagnostic). WDYT?

[dblaikie]

clang/include/clang/AST/Stmt.h

596–597

"too complex" seems like totally the right phrasing to me

[yronglin]

clang/include/clang/AST/Stmt.h

596–597

Thanks for your suggestions, I have updated D158296

clang/docs/ReleaseNotes.rst

 - Fix the contains-errors bit not being set for DeclRefExpr that refers to a
   VarDecl with invalid initializer. This fixes:
   (`#50236 <https://github.com/llvm/llvm-project/issues/50236>`_),
   (`#50243 <https://github.com/llvm/llvm-project/issues/50243>`_),
   (`#48636 <https://github.com/llvm/llvm-project/issues/48636>`_),
   (`#50320 <https://github.com/llvm/llvm-project/issues/50320>`_).
 - Fix an assertion when using ``\u0024`` (``$``) as an identifier, by disallowing
   that construct (`#62133 <https://github.com/llvm/llvm-project/issues/38717>_`).
+- Fix crash caused by PseudoObjectExprBitfields: NumSubExprs overflow.
+  (`#63169 <https://github.com/llvm/llvm-project/issues/63169>_`)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/include/clang/AST/Stmt.h

protected:
   };
 
   class PseudoObjectExprBitfields {
     friend class ASTStmtReader; // deserialization
     friend class PseudoObjectExpr;
 
     unsigned : NumExprBits;
 
-    // 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;
   };
 
   class SourceLocExprBitfields {
     friend class ASTStmtReader;
     friend class SourceLocExpr;
 
     unsigned : NumExprBits;
 

clang/test/SemaCXX/builtin-dump-struct.cpp

void errors(B b) {
   __builtin_dump_struct(1); // expected-error {{too few arguments to function call, expected 2, have 1}}
   __builtin_dump_struct(1, 2); // expected-error {{expected pointer to struct as 1st argument to '__builtin_dump_struct', found 'int'}}
   __builtin_dump_struct(&b, 2); // expected-error {{expected a callable expression as 2nd argument to '__builtin_dump_struct', found 'int'}}
   __builtin_dump_struct(&b, Format, 0); // expected-error {{no matching function for call to 'Format'}}
                                         // expected-note@-1 {{in call to printing function with arguments '(0, "%s", "B")' while dumping struct}}
                                         // expected-note@#Format {{no known conversion from 'int' to 'ConstexprString &' for 1st argument}}
 }
 #endif
+
+// Check that PseudoObjectExprBitfields:NumSubExprs doesn't overflow. This
+// would previously cause a crash.
+struct t1 {
+  int v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+      v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+      v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+      v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+      v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+      v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+      v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+struct t2 {
+  t1 v0, v1, v2, v3, v4, v5, v6, v7, v8, v9, v10, v11, v12, v13, v14, v15, v16,
+      v17, v18, v19, v20, v21, v22, v23, v24, v25, v26, v27, v28, v29, v30, v31,
+      v32, v33, v34, v35, v36, v37, v38, v39, v40, v41, v42, v43, v44, v45, v46,
+      v47, v48, v49, v50, v51, v52, v53, v54, v55, v56, v57, v58, v59, v60, v61,
+      v62, v63, v64, v65, v66, v67, v68, v69, v70, v71, v72, v73, v74, v75, v76,
+      v77, v78, v79, v80, v81, v82, v83, v84, v85, v86, v87, v88, v89, v90, v91,
+      v92, v93, v94, v95, v96, v97, v98, v99;
+};
+
+int printf(const char *, ...);
+void f1(t2 w) { __builtin_dump_struct(&w, printf); }