tools: rimage: return a negative error code from elf_error() - #11077
tools: rimage: return a negative error code from elf_error()#11077abonislawski wants to merge 1 commit into
Conversation
elf_error() negates its argument, so an already negative code was turned into a positive value that callers testing "ret < 0" take for success. Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
There was a problem hiding this comment.
Pull request overview
Fixes an error-sign bug in tools/rimage where elf_error() could return a positive value when given an already-negative error code, causing callers that check ret < 0 to misinterpret failures as success.
Changes:
- Update
elf_error()to preserve negative error codes while still negating positiveerrnovalues. - Clarify the
elf_error()Doxygen comment to document the expected sign of inputs/outputs.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Documentation doesn't quite match the code change....?
| * @param msg error message | ||
| * @param error error code to return | ||
| * @return error code | ||
| * @param error positive errno value, or an already negative error code |
There was a problem hiding this comment.
Hmm, code on L104 seems to always return a negative number or zero), so this documentation seems wrong. positive errno is never returned.
Callers seem to check for non-zero, so maybe this should be "0 on success", or keep the code logic and "0 on success, negative error"
There was a problem hiding this comment.
This is about param which could be positive (and it is in one single call).
The next line is about return value (negative error code).
In this path caller checks only <0 (rimage.c#L249), this could be changed but anyway elf_error return -ENOMEM in elf_section_read, and then its again switched from -ENOMEM to ENOMEM in elf_strings_read_by_index.
Its just stupid and must to be fixed somewhere so I decided to make a simple check to avoid inconsistent behavior (negative or positive error code)
elf_error() negates its argument, so an already negative code was turned into a positive value that callers testing "ret < 0" take for success.