prepare-root: Properly check return value of snprintf()#3568
prepare-root: Properly check return value of snprintf()#3568ukleinek wants to merge 1 commit intoostreedev:mainfrom
Conversation
|
Hi @ukleinek. Thanks for your PR. I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a potential bug in snprintf usage by adding a check for truncation, in addition to the existing error check. This prevents the use of potentially truncated paths, improving the robustness and security of the ostree-prepare-root-static tool. The changes are correct and well-implemented. I've added a few suggestions to further improve the error handling by using errx instead of err to avoid misleading error messages on truncation, and to make the signed/unsigned comparison explicit.
I also noticed a similar incorrect snprintf check in src/switchroot/ostree-prepare-root.c at line 121. It would be great to address that as well in a follow-up to ensure consistency.
|
The "Debian Testing" failure is due to a preparing step failing and isn't triggered by the changes in this PR. (fuse was removed from testing, see https://tracker.debian.org/pkg/fuse) |
When the target buffer is to small to hold the resulting string a value larger or equal than the buffer's size is returned. (In pre C99 versions, snprintf returned -1 in this case, too.) So to ensure that no truncated paths are used adapt the error checking accordingly.
|
I don't understand the failing continuous-integration/jenkins/pr-merge check, but I suspect it is also independent of my changes. Please tell me if I'm wrong here. |
When the target buffer is to small to hold the resulting string a value larger or equal than the buffer's size is returned. (In pre C99 versions, snprintf returned -1 in this case, too.)
So to ensure that no truncated paths are used adapt the error checking accordingly.
The other usages of snprintf either use a correct check or none at all.