[DO NOT MERGE] All of the changes we made for the ff integration#544
Draft
randomPoison wants to merge 8 commits intofw/fffrom
Draft
[DO NOT MERGE] All of the changes we made for the ff integration#544randomPoison wants to merge 8 commits intofw/fffrom
randomPoison wants to merge 8 commits intofw/fffrom
Conversation
* Differentiate between decls that shouldn't be rewritten vs ones that should not be analyzed at all. * Only rewrite files files in the rewrite set when in library only mode. * Avoid overwriting unchanged files when copying files into the rewrite dir. This helps avoid needing to recompile everything after running the rewriter.
* Add `extern "C"` blocks around various definitions so that they can be included in C++ code. * Fix various places where C++ semantics don't line up with our C code, e.g. C allows implicit casts between pointer types, but in C++ we need to cast to `char*` manually. * Other changes that I don't remember the rational for.
ayrtonm
reviewed
May 2, 2025
| } | ||
| printf("TLS segment of %s is not padded\n", libname); | ||
| exit(-1); | ||
| // exit(-1); |
Contributor
There was a problem hiding this comment.
Did we end up needing this change?
Contributor
Author
There was a problem hiding this comment.
Yeah, currently our various libs are not padded, so this causes the ff to exit. We could probably start padding those and then we can put this exit() back.
randomPoison
commented
May 7, 2025
Comment on lines
+437
to
+467
| #ifdef IA2_NO_DEFS | ||
| #define _IA2_INIT_RUNTIME(n) \ | ||
| __attribute__((visibility("default"))) extern int ia2_n_pkeys_to_alloc; \ | ||
| __attribute__((visibility("default"))) extern __thread void *ia2_stackptr_0[] \ | ||
| __attribute__((aligned(4096))); \ | ||
| \ | ||
| REPEATB(n, declare_init_tls_fn, nop_macro); \ | ||
| \ | ||
| /* Returns `&ia2_stackptr_N` given a pkru value for the Nth compartment. */ \ | ||
| __attribute__((visibility("default"))) void **ia2_stackptr_for_pkru(uint32_t pkru) { \ | ||
| REPEATB(n, return_stackptr_if_compartment, \ | ||
| return_stackptr_if_compartment); \ | ||
| return NULL; \ | ||
| } \ | ||
| \ | ||
| __attribute__((visibility("default"))) __attribute__((weak)) void init_stacks_and_setup_tls(void) { \ | ||
| verify_tls_padding(); \ | ||
| COMPARTMENT_SAVE_AND_RESTORE(REPEATB(n, ALLOCATE_COMPARTMENT_STACK_AND_SETUP_TLS, nop_macro), n); \ | ||
| /* allocate an unprotected stack for the untrusted compartment */ \ | ||
| allocate_stack_0(); \ | ||
| } \ | ||
| \ | ||
| __attribute__((constructor)) static void ia2_init(void) { \ | ||
| /* Set up global resources. */ \ | ||
| ia2_set_up_tags(&ia2_n_pkeys_to_alloc); \ | ||
| /* Initialize stacks for the main thread/ */ \ | ||
| init_stacks_and_setup_tls(); \ | ||
| REPEATB##n(setup_destructors_for_compartment, nop_macro); \ | ||
| mark_init_finished(); \ | ||
| } | ||
| #else |
Contributor
Author
There was a problem hiding this comment.
@ayrtonm @fw-immunant Do either of you remember what this was about? We don't seem to be using IA2_NO_DEFS anywhere else, so I'm guessing it's not relevant anymore.
This fixes an issue if ff where we would hit a compartment violation inside of static constructors. When we loading libxul at startup, the init_pkey would get called and would mark the global memory as part of compartment 1, and then would set the pkey to 0. Then the static constructor would run and hit a compartment violation for trying to access compartment 1 memory from compartment 0. This change clears the pkru after initialization, allowing the static constructor to access compartment 1 memory. This is basically the same fix as #561.
By default the thread isolated pools only get ~256 Mb of address space, whereas the normal pools get ~18 Gb. Since the thread isolated pools are the only ones used when compartmentalized, this caused Firefox to quickly run out of address space and hit OOM crashes.
Without explicitly adding `visibility("default")` the symbols were coming up as hidden in ff. This is the same issue we ran into elsewhere where we had to explicility mark visibility.
Firefox also has some cases where it uses `strdup` to copy a string and then tries to access that string from within libjpeg (via environment variables), so I've added `shared_strdup`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.