-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
elf: Fix for mismatched app ELF file not detected. (IDFGH-9855) #3
elf: Fix for mismatched app ELF file not detected. (IDFGH-9855) #3
Conversation
If possible, I'd appreciate a backport to the ESP-IDF release/v4.4 branch as this is where I stumbled across it. I hope you folks are all doing well. :) |
40c7383
to
a9ec52c
Compare
Hmm OK, this now fails for another reason. The core dump I have only contains the first 16 characters of the ELF SHA, for some reason:
The remaining bytes are there, but all zero. EDIT:
ah yeah,that'll do it. |
The check that the app ELF file SHA256 matches the one stored in the core dump would never fail, leading to gdb loading the wrong ELF file and either crashing or producing misleading debug information. Specifics: The note_sec.name field was incorrectly read back as b'ESP_CORE_DUMP_INFO\x00E', because the namesz length includes the terminating NUL byte and possible junk padding bytes: https://github.com/espressif/esp-idf/blob/master/components/espcoredump/src/core_dump_elf.c#L212 In addition, as 'note_sec.name' is a bytes object Python 3 would have never successfully compared it with a string.
a9ec52c
to
3cb9df5
Compare
Hey @projectgus, thanks for the fix!
(I now see your updated comment) |
@igrr yeah, I just remembered/saw this now as well! So I guess the Python code needs to compare all the bytes in the core dump file, except for any trailing 00 bytes? |
I think so, yes... Ideally we should have also recorded the length of the truncated SHA256 there, but alas. Over here we use which is not very precise, but i guess the likelihood that the truncated SHA256 is a substring of a different SHA256 is pretty low in practice. |
The comment says it returns the "SHA256 hash of the input ELF file", but this is not true - it was the SHA256 hash of the output ELF file. As the parser may change some bytes around in minor ways, these were often not the same.
With the default APP_RETRIEVE_LEN_ELF_SHA setting, core dump files only have a truncated ELF SHA256 in them. Account for this when comparing the core dump SHA with the app ELF SHA.
Thanks for the explanation, @igrr. Have pushed some fixes that seem to allow esp-coredump.py to detect correctly if the APP ELF matches or not. This fix may potentially save a lot of hair-pulling about "nonsense" core dump debug output! |
@projectgus I have done the backport of your fixes to IDF release/v4.4, but it turned out the ELF & coredump files used in our CI actually didn't have matching SHA256. (Which is understandable, since these were just "dummy" ELF files, but it never occurred to me that such combination is not supposed to work.) |
No worries @igrr, thanks for the update! |
Thank you @projectgus for the contribution. It revealed a couple of issues in our tests which had to fixed first. That is why this took long to solve. Now it is merged to the v4.4 release branch and should be available soon. Probably it will be released in v4.4.6. Here are the commits in v4.4 for future reference: |
No worries at all @dobairoland, thanks for the update! |
The check that the app ELF file SHA256 matches the one stored in the core dump would never fail, leading to gdb loading the wrong ELF file and either crashing or producing misleading debug information.
Specifics:
The
note_sec.name
field was incorrectly read back asb'ESP_CORE_DUMP_INFO\x00E'
, because thenamesz
length includes the terminating NUL byte and possible junk padding bytes:https://github.com/espressif/esp-idf/blob/master/components/espcoredump/src/core_dump_elf.c#L212
In addition, as
note_sec.name
is a bytes object Python 3 would have never successfully compared it with a string.