[PATCH] D121556: [randstruct] Add randomize structure layout support

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Randstruct.h:34
+
+extern std::string SEED;
+
----------------
void wrote:
> aaron.ballman wrote:
> > Doing this with a global variable is unfortunate; it could make things harder when we multithread the frontend. Can we solve this without a global? (And if not, does this global need to be a `ManagedStatic`?)
> Maybe this could be moved to `LangOpts`?
That's a really good idea, let's go with that approach.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11543
+// Layout randomization warning.
+def cast_from_randomized_struct : Error<
+  "casting from randomized structure pointer type %0 to %1">;
----------------



================
Comment at: clang/lib/AST/Randstruct.cpp:99
+
+    if (FD->isBitField() && !FD->isZeroLengthBitField(Context)) {
+      // Start a bitfield run if this is the first bitfield we have found.
----------------
void wrote:
> aaron.ballman wrote:
> > Oh cool, we do bit-field runs!
> > 
> > How should we handle anonymous bit-fields (if anything special should be done for them at all)? People usually use them for padding between bit-fields, so it's not clear to me how to treat them when randomizing.
> Good question! I'm not sure either. On one hand, I'm rather concerned about changing bitfield order in general, but it appears to be something that GCC's plugin does, so...
> 
Yeah, I was really surprised about bit-fields as well. I'm especially concerned that bit-fields which were [un]carefully straddling allocation unit boundaries might be rearranged such that subtle atomic bugs and whatnot will be exposed.

If we find these sort of concerns are valid in real world code, we may want to add a second command line option (off-by-default, perhaps) for enabling rearranging bit-fields. But for now, I think it's fine to follow GCC's lead here.


================
Comment at: clang/lib/AST/Randstruct.cpp:179
+
+  SmallVector<FieldDecl *, 64> RandomizedFields;
+
----------------
void wrote:
> aaron.ballman wrote:
> > This one seems a bit large to me; any reason not to use `16` again?
> The code above looks to apply mostly to bitfield runs. This is for all fields in a structure. I assumed (without proof) that this will always be larger than the bitfield runs. :-)
I think that's a safe assumption and it's probably not worth worrying about overly much. It's more that 16 bit-fields seemed like it would be large enough to cover most bit-fields in a class while still being "small", but 64 fields seems likely to be way larger than most structures will need and isn't very small.

That said, I don't have a strong feeling here and I think the numbers are defensible unless we measure something that says otherwise.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2503
+
+  // FIXME: Any way to get a handle to a RecordDecl struct here?
+  clang::randstruct::checkForBadCasts(S, AC);
----------------
void wrote:
> aaron.ballman wrote:
> > No, this function is only called when popping a function scope, and so the only declaration it has access to is the `FunctionDecl`. Or do I misunderstand the question?
> Sounds good to me. Is this the best place for this check then?
I'm not convinced it is the best place for it -- analysis-based warnings are typically ones that require a CFG to be analyzed and so they are only run once we know the function body itself is valid (they also typically require the user to manually enable the checks -- your check runs always, even if the user hasn't enabled struct randomization, so it walks a lot of ASTs it doesn't need to). Your check doesn't require a CFG at all and it seems like it can be done purely when building the AST. I think a better place for this checking is in SemaCast.cpp in the `CastOperation` class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556