aaron.ballman added a comment.
Thank you for working on this new security feature! I like it and I think it's
heading in the right direction.
Can you please add a release note for the new functionality?
This is missing tests for the Driver diagnostics, the Sema diagnostics
(including existing diagnostics like writing the attribute on the wrong subject
or giving it args when it doesn't expect them, wrong language mode, etc), and
the CodeGen behavior, so those should be added. Also, the Windows pre-commit CI
is failing:
Failed Tests (7):
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.AnonymousStructsAndUnionsRetainFieldOrder
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.CheckAdjacentBitfieldsRemainAdjacentAfterRandomization
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.CheckVariableLengthArrayMemberRemainsAtEndOfStructure
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.MultipleAttrsRandomize
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.RandstructDoesNotOverrideThePackedAttr
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.ZeroWidthBitfieldsSeparateAllocationUnits
Testing Time: 676.49s
Skipped : 4
Unsupported : 202
Passed : 29832
Expectedly Failed: 31
Failed : 7
================
Comment at: clang/include/clang/AST/Decl.h:2842
mutable unsigned CachedFieldIndex : 30;
+ mutable unsigned OriginalFieldIndex : 30;
----------------
It's unfortunate that every field node in the AST is now going to be 4 bytes
larger for this feature; I worry about the extra memory pressure from the
additional overhead, so if there's a way for us to save some space here, I
think it might be worth it. (I'm worried that our max template instantiation
depth will shrink because of this.)
================
Comment at: clang/include/clang/AST/Decl.h:4067
+
+ void setIsRandomized(bool V) const { RecordDeclBits.IsRandomized = V; }
+
----------------
A setter that is marked `const` does not spark joy for me... Can we use
friendship rather than mutability to solve this?
================
Comment at: clang/include/clang/AST/Randstruct.h:34
+
+extern std::string SEED;
+
----------------
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`?)
================
Comment at: clang/include/clang/Basic/Attr.td:3938-3939
+def RandomizeLayout : InheritableAttr {
+ let Spellings = [GCC<"randomize_layout">, Declspec<"randomize_layout">,
+ Keyword<"randomize_layout">];
+ let Subjects = SubjectList<[Record]>;
----------------
Microsoft does not implement this `__declspec`, correct? If they don't, we
shouldn't either (even if GCC does) unless there's a *very* good reason to do
so. That's Microsoft's design space we're encroaching on.
I'd also like to understand the justification for adding a new keyword for this.
================
Comment at: clang/include/clang/Basic/Attr.td:3948-3949
+def NoRandomizeLayout : InheritableAttr {
+ let Spellings = [GCC<"no_randomize_layout">, Declspec<"no_randomize_layout">,
+ Keyword<"no_randomize_layout">];
+ let Subjects = SubjectList<[Record]>;
----------------
Same here.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:6367
+program should be compiled with the same seed, but keep the seed safe
+otherwise.
+
----------------
I think it would help to explain the effects of this attribute in conjunction
with structure packing techniques (the `packed` attribute and bit-fields) and
whether any attempt is made to keep the structure packed or those fields
together or not. I'd expect no such attempt is made, but people may want to
know "attempts to pack this structure" and "randomize this structure layout"
are not compatible. (We may want to diagnose in the case of seeing the `packed`
attribute, in fact.)
We should also be more clear that the seed specified does not have to be
numeric in nature (from the file or when directly on the command line).
================
Comment at: clang/lib/AST/DeclBase.cpp:28
#include "clang/AST/ExternalASTSource.h"
+#include "clang/AST/Randstruct.h"
#include "clang/AST/Stmt.h"
----------------
Is this include necessary?
================
Comment at: clang/lib/AST/Randstruct.cpp:72
+ // All of the Buckets produced by best-effort cache-line algorithm.
+ // TODO: Figure out a better initial size.
+ SmallVector<std::unique_ptr<randstruct::Bucket>, 16> Buckets;
----------------
This size seems as defensible as most others; do you plan to do more here, or
should this comment be removed?
================
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.
----------------
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.
================
Comment at: clang/lib/AST/Randstruct.cpp:179
+
+ SmallVector<FieldDecl *, 64> RandomizedFields;
+
----------------
This one seems a bit large to me; any reason not to use `16` again?
================
Comment at: clang/lib/AST/Randstruct.cpp:185
+ ++TotalNumFields;
+ if (auto FD = dyn_cast<FieldDecl>(D)) {
+ FD->setOriginalFieldIndex(Index++);
----------------
================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2063-2069
+ if (RD->isRandomized()) {
+ for (Decl *D : FinalOrdering)
+ RD->removeDecl(D);
+
+ for (Decl *D : FinalOrdering)
+ RD->addDecl(D);
+ }
----------------
I think this works okay for C, but I think if we ever attempted to provide this
functionality in C++, the calls to `addDecl()` would be rather expensive
because it eventually calls `addedMember()` which calculates information about
the structure (whether it's copy constructible, has a trivial destructor, etc)
and we don't need to redo any of that work.
However, for now, I think this is fine.
================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2166-2167
+ if (isa<PointerType>(ExprTy) && isa<PointerType>(SubExprTy)) {
+ const QualType CastTy = cast<PointerType>(ExprTy)->getPointeeType();
+ const QualType BaseTy = cast<PointerType>(SubExprTy)->getPointeeType();
+
----------------
================
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);
----------------
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?
================
Comment at: clang/lib/Sema/SemaDecl.cpp:27
#include "clang/AST/NonTrivialTypeVisitor.h"
+#include "clang/AST/Randstruct.h"
#include "clang/AST/StmtCXX.h"
----------------
Is this include necessary?
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8550-8556
+ case ParsedAttr::AT_RandomizeLayout:
+ handleSimpleAttribute<RandomizeLayoutAttr>(S, D, AL);
+ break;
+ case ParsedAttr::AT_NoRandomizeLayout:
+ // Drop the "randomize_layout" attribute if it's on the decl.
+ D->dropAttr<RandomizeLayoutAttr>();
+ break;
----------------
I don't think this is sufficient. Consider redeclaration merging cases:
```
struct [[gnu::randomize_layout]] S;
struct [[gnu::no_randomize_layout]] S {};
struct [[gnu::no_randomize_layout]] T;
struct [[gnu::randomize_layout]] T {};
```
I think if the user accidentally does something like this, it should be
diagnosed. I would have assumed this would warrant an error diagnostic because
the user is obviously confused as to what they want, but it seems GCC ignores
the subsequent diagnostic with a warning: https://godbolt.org/z/1q8dazYPW.
There's also the "confused attribute list" case which GCC just... does...
things... with: https://godbolt.org/z/rnfsn7hG1. I think we want to behave more
consistently with how we treat these cases.
Either way, we shouldn't be silent.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121556/new/
https://reviews.llvm.org/D121556
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
