rjmccall kcc rsmith
- rC346001: Change -fsanitize-address-poison-class-member-array-new-cookie to -fsanitize…
rG0eb5008352d8: Change -fsanitize-address-poison-class-member-array-new-cookie to -fsanitize…
rL346001: Change -fsanitize-address-poison-class-member-array-new-cookie to -fsanitize…
I strongly dislike this one because "poison array cookie", in general, is always enabled (it's actually triggered by a runtime flag). This flag is about poisoning it in more cases (cases where the standard doesn't completely disallow accesses to the cookie, so we have to have a flag and can't enable it all the time).
P.S: Some additional discussion is at D41664, from when this flag was first implemented.
Hmm. This naming discussion might be a proxy for another debate. I understand the argument that programs are allowed to assume a particular C++ ABI and therefore access the array cookie. And I can understand having an option that makes ASan stricter and disallow accessing the array cookie anyway. But I don't understand why this analysis is in any way case-specific, and the discussion you've linked doesn't seem particularly clarifying about that point. Can you explain this a little?
If you don't have a class-member operator new, then you don't expect to be able to have access to the memory occupied by the array cookie, since you never "see" a pointer that points to it.
But if you have a class-member operator new, then you can keep a copy of all the pointers you've returned, and you're allowed to read from them, even if they contain an array cookie (you're just reading from memory you've returned to users).
With this flag, we disallow this second case, at the expense of having ASan trigger an error on code that is standards compliant. We're making it more strict, therefore we start emitting ASan errors on the test I was removing in D41664. Kostya's comment on me removing that test was that the code is valid, so ASan shouldn't reject it by default (but ok to have flags that make it more strict).
P.S: We still have "placement new" (::operator new(size_t, void*)as a special case, which will not have an array cookie, so we don't poison it. This flag only applies to user-defined, class-specific allocation functions.
P.P.S: Sorry if I misunderstood your question. Please let me know.
So, three points:
- That's not class-member-specific; you can have a placement operator new at global scope that isn't the special void* placement operator and therefore still has a cookie, and it would have just as much flexibility as a class-member override would. So the split you're trying to describe is the standard operators vs. custom ones.
- Anyone can provide their own definition of the standard operators; there are some semantic restrictions on those definitions, but I'm not sure what about those restrictions would forbid this kind of capture.
- Even with the standard implementations of the standard replaceable operators, I'm not sure what rule would actually outlaw the client from going from the result of new back to the cookie.
At any rate, taking the feature as a given, the first point suggests -fsanitize-address-poison-custom-array-cookie or something along those lines. If we want a more standard-ese term than "custom", the standard refers to its operators collectively as "library allocation functions", so maybe "non-library".
Thank you. I went with your suggestion, as I think it's close enough to be understandable. I've also changed the -cc1 argument in this patch, as it looks weird to have two different arguments (unless needed). Let me know if you'd like to keep the different names.
Thanks, and I agree with renaming the existing option.
|805 ↗||(On Diff #171661)|
This is user documentation, so it would be good to explain here what exactly this does and why you might enable or disable it. I know the surrounding documentation is even more barebones, but that's a problem, and it's a problem we won't fix by making it worse.
|805 ↗||(On Diff #171661)|
Thanks. Grammar/style clean-up: