Introduced a PRIVATE(member) macro #4409
Conversation
mstarzyk-mobica
force-pushed
the
private_fields_test
branch
from
d79b6c6 to
2b96a3d
Compare
mstarzyk-mobica
changed the title
mstarzyk-mobica
force-pushed
the
private_fields_test
branch
7 times, most recently
from
c6eab60 to
37939f0
Compare
mstarzyk-mobica
force-pushed
the
private_fields_test
branch
3 times, most recently
from
c6272e9 to
c42aeac
Compare
private structures' members a bit harder. Signed-off-by: Mateusz Starzyk <mateusz.starzyk@mobica.com>
mstarzyk-mobica
force-pushed
the
private_fields_test
branch
from
c42aeac to
be9a842
Compare
gilles-peskine-arm
added
needs-design-approval
needs-reviewer
| @@ -99,7 +105,7 @@ typedef struct mbedtls_md_context_t | |||
| const mbedtls_md_info_t *md_info; | |||
|
|
|||
| /** The digest-specific context. */ | |||
| void *md_ctx; | |||
| void *MBEDTLS_PRIVATE(md_ctx); | |||
There was a problem hiding this comment.
This is likely to confuse source code navigation. I don't know if this really matters: do we often look up field names (as opposed to function or type names) when working on the library?
There was a problem hiding this comment.
I can't speak for others, but I never look up field names. So I don't think the disruption to source code navigation is a big issue here.
mstarzyk-mobica
force-pushed
the
private_fields_test
branch
2 times, most recently
from
780a455 to
ab5b02a
Compare
Instead of defining MBEDTLS_ALLOW_PRIVATE_ACCESS inside makefiles, use 'common.h' header. Move access restriction macro to dedicated header file: private_access.h Use MBEDTLS_ALLOW_PRIVATE_ACCESS macro for both MBEDTLS and PSA. Signed-off-by: Mateusz Starzyk <mateusz.starzyk@mobica.com>
mstarzyk-mobica
force-pushed
the
private_fields_test
branch
from
ab5b02a to
888c16c
Compare
| /** Allow library to access it's structs' private members. | ||
| * | ||
| * Although structs defined in header files are publicly available, | ||
| * their members are private and should not be accessed by the user. |
There was a problem hiding this comment.
just a short note for now, later on when this task is fully implemented, this will need more in-depth explanation
Signed-off-by: Mateusz Starzyk <mateusz.starzyk@mobica.com>
| #define MBEDTLS_PRIVATE_ACCESS_H | ||
|
|
||
| #ifndef MBEDTLS_ALLOW_PRIVATE_ACCESS | ||
| #define MBEDTLS_PRIVATE(member) private_##member |
There was a problem hiding this comment.
Changing the name of struct members is not guaranteed to result in compatible object files. I expect this works with most compilers when invoked in an ordinary way, but does it work with link-time optimization? And even if it does, are we comfortable taking the risk that it will break in some environments, perhaps in subtle ways resulting in access to the wrong field (although most likely breaking with a link error)?
There was a problem hiding this comment.
Can you elaborate how changing the name of a struct member could, possibly and/or realistically, result in incompatible object files?
There was a problem hiding this comment.
In theory, as far as I can tell from reading the C standard (i.e. I haven't found a provision to the contrary), a compiler could base padding on the field names and not just on the field types. (Not the order: a field always has to be at a higher address than the previous field.) In other words, given
struct public { a_t a; b_t b; c_t c; };
struct private { a_t private_a; b_t private_b; c_t private_c; };
it's possible, but “exotic”, that offsetof(struct private, private_b) != offsetof(struct public, private_b). The C standard only guarantees that offsetof(…a) == 0 and offsetof(…b) >= sizeof(a_t) and offsetof(…c) >= offsetof(…b) + sizeof(b_t).
I can't think of a plausible reason why a compiler would do this, but it could. Maybe some build instrumented for runtime debugging that includes the field name after each field?
For bit-fields, N1256 states “The order of allocation of bit-fields within a unit (high-order to low-order or low-order to high-order) is implementation-defined”. So something like ordering field names by hash value would not be permitted, but basing the choice between low-to-high or high-to-low on parity of the total number of letters in field names would be as long as it's documented (but of course it would be really bizarre).
With link-time optimization, the link-time optimizer may try to access struct members and error out because a field doesn't exist. I have not checked any compiler's LTO.
There was a problem hiding this comment.
I've now checked LTO with GCC 10 and Clang 10. This PR passes the unit tests.
There was a problem hiding this comment.
Yes, I had a similar concern, but then thought (1) I think it's unlikely to matter in practice for common compilers with common options, and (2) even if it happens with some compiler/option, I think people using those exotic settings can always define MBEDTLS_ALLOW_PRIVATE_ACCESS on the command line to get rid of the renaming.
Again, the point of this is not to fully prevent users from accessing private fields, but to help them notice if they do it inadvertently. I think it's OK for people using hypothetical exotic build settings to not have access to that layer of protection. (And actually, they could still in their CI do an extra build with more standard settings to catch any unintentional uses of private fields, since this would be compile-time error.)
Thanks for checking what the standard says and how it works with LTO.
gilles-peskine-arm
added
approved for design
DO-NOT-MERGE
and removed
needs-design-approval
needs-ci
|
Following a team discussion, we've decided to go ahead with this approach. The next step is to apply this change to (almost) every field in a public structure. It will be easier to review the changes if they are applied in some systematic way, rather than by manual editing where we have to review every single line. So we will probably not merge this pull request as it is, but rather a cherry-pick of commits that affect build scripts and introduce the new macro, and a different commit structure for the commits that modify existing library files. |
ronald-cron-arm
removed
the
needs-reviewer
|
#4511 now contains all the relevant changes from this PR, plus much more, so I'm closing this one. |
solution proposal for #4372