Add better error reporting in the ELF loader #624
Conversation
Replaced ptr_cleanup label Replaced *sz labels
reynir
left a comment
reynir
left a comment
There was a problem hiding this comment.
Thanks!
I think this adds a lot of gotos (and labels!) which maybe goes a bit off from the current style. Since most of the labels are warnx("..."); goto mem_cleanup I wonder if it could be rewritten so the source locations of the gotos instead call warnx("...") and then goto mem_cleanup? Then we have fewer locations we goto.
|
Thank you for the feedback, I'll adjust the relevant lines immediately to reduce the bloating. |
| if (ehdr->e_entry < p_min_loadaddr || ehdr->e_entry >= mem_size) | ||
| goto out_invalid; | ||
| if (ehdr->e_entry < p_min_loadaddr || ehdr->e_entry >= mem_size) { | ||
| warnx("%s: e_entry does not fall within the memory allocation range", bin_name); |
There was a problem hiding this comment.
Reading this again this error message is very specific and perhaps lacks a bit of context. So if the executable seems invalid or unsupported we say so and then go into further detail rather than diving deep into details. I think for people not deep into ELF will have a hard time understand why the program fails with such a message.
| warnx("%s: e_entry does not fall within the memory allocation range", bin_name); | |
| warnx("%s: Invalid or unsupported executable: e_entry does not fall within the memory allocation range", bin_name); |
There was a problem hiding this comment.
Hi @reynir, thank you for your feedback. I had a look at the rest of the error messages, in particular do you think that the messages listed below will benefit from these changes as well?
- "p_vaddr_start is outside of memory range"
- "note does not fall within valid size"
3 participants
Added a few extra goto error messages relating to elf headers, program headers, and note headers have been added in relation to #396.