i#7823: Look for MEMPROT_EXEC when looking for the executable#7824
i#7823: Look for MEMPROT_EXEC when looking for the executable#7824palmer-dabbelt wants to merge 1 commit intoDynamoRIO:masterfrom
Conversation
| } | ||
| # endif | ||
| if (found_exec) { | ||
| if (found_exec && TEST(MEMPROT_EXEC, iter->prot)) { |
There was a problem hiding this comment.
The PR description mentions there were some ASSERT crashes that are fixed by this change (I assume you were referring to the L10307 check). Can you check why found_exec was set to true in that case? E.g., was it that the L10273 comparison found a region named exactly like get_application_name() that's somehow not executable? Or is it the anonymous region for the header (L10274; but executable_start == iter->vm_start should be true already for that case)?
There was a problem hiding this comment.
Folly's symbolizer maps the executable a second time as read-only. I don't know the specifics of why it feels it needs to do that, but here's the code: https://github.com/facebook/folly/blob/93187f748cc5d9aebdffa58e462c146a27cfb713/folly/debugging/symbolizer/Elf.cpp#L108 .
FWIW: it's also using MAP_SHARED, which seems odd, but MAP_PRIVATE has the same behavior in DynamoRIO so at least that's not the issue.
There was a problem hiding this comment.
I see. Can we augment the L10273 comparison to also check for MEMPROT_EXEC? That seems better to me as it avoids changing the handling for the anon header case.
There was a problem hiding this comment.
Ya, if you think that's safe then it's probably the better bet.
I don't know the larger context of what's going on here, but just from looking at the place of the assent it seems somewhat sane to be checking for MEMPROT_EXEC here (or possibly even above where we're looking for MEMPROT_READ) -- we're looking for the executable, so the mapping must be executable. This makes the assert stop triggering for us, but I'm not sure if it has some other issues. Fixes DynamoRIO#7823 Signed-off-by: Palmer Dabbelt <palmerdabbelt@meta.com>
|
Please don't force push shared branches; see https://dynamorio.org/page_code_reviews.html#sec_code_review_non_member and https://dynamorio.org/page_code_reviews.html#autotoc_md118. |
oh sorry, I'll read the docs. Do you want me to reset the branch to where it was? |
| image = true; | ||
| DODEBUG({ map_type = "ELF SO"; }); | ||
| } else if (TEST(MEMPROT_READ, iter->prot) && | ||
| } else if (TEST(MEMPROT_READ | MEMPROT_EXEC, iter->prot) && |
There was a problem hiding this comment.
I thought the consensus in the other comment thread was to add this MEMPROT_EXEC check to L10288 (the line #s have changed now it seems; maybe because the force-push also updated the branch baseline?). Like I said, by doing that, we can avoid making any assumption about the anon region case (see L10289).
Does this solve your crash?
Not necessary. The changes in this PR are simple enough that losing that history didn't make reviewing much harder. |
2 participants
I don't know the larger context of what's going on here, but just from looking at the place of the assent it seems somewhat sane to be checking for MEMPROT_EXEC here (or possibly even above where we're looking for MEMPROT_READ) -- we're looking for the executable, so the mapping must be executable.
This makes the assert stop triggering for us, but I'm not sure if it has some other issues.
Fixes #7823