Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd: use safer functions in sc_mount_opt2str #2778
Conversation
zyga
added some commits
Feb 3, 2017
All checks have passed
| @@ -32,6 +32,7 @@ int main(int argc, char *argv[]) | ||
| fprintf(stderr, "cannot parse given argument as a number\n"); | ||
| return 1; | ||
| } | ||
| - printf("%#lx is %s\n", mountflags, sc_mount_opt2str(mountflags)); | ||
| + char buf[1000]; | ||
| + printf("%#lx is %s\n", mountflags, sc_mount_opt2str(buf, sizeof buf, mountflags)); |
mvo5
Feb 7, 2017
Collaborator
(nitpick) I noticed we have this a lot in the code but I personally prefer the parenthese around sizeof(buf). I know its not strictly needed but sometimes it and sometimes it is not and thats why I prefer consistency (but not a blocker if everyone else is fine with this).
zyga
Feb 7, 2017
Contributor
Ha, this is a weird part of C syntax: sizeof expr does not need parentheses but sizeof(type) does.
zyga
requested a review
from
jdstrand
Feb 7, 2017
jdstrand
reviewed
Feb 7, 2017
Thanks for this PR-- liking the cleanups here. LGTM with the one small future-proofing comment (and again, +1 on 'sizeof(...)' for consistency).
| @@ -32,6 +32,7 @@ int main(int argc, char *argv[]) | ||
| fprintf(stderr, "cannot parse given argument as a number\n"); | ||
| return 1; | ||
| } | ||
| - printf("%#lx is %s\n", mountflags, sc_mount_opt2str(mountflags)); | ||
| + char buf[1000]; | ||
| + printf("%#lx is %s\n", mountflags, sc_mount_opt2str(buf, sizeof buf, mountflags)); |
mvo5
Feb 7, 2017
Collaborator
(nitpick) I noticed we have this a lot in the code but I personally prefer the parenthese around sizeof(buf). I know its not strictly needed but sometimes it and sometimes it is not and thats why I prefer consistency (but not a blocker if everyone else is fine with this).
zyga
Feb 7, 2017
Contributor
Ha, this is a weird part of C syntax: sizeof expr does not need parentheses but sizeof(type) does.
| } | ||
| // Chop the excess comma from the end. | ||
| - size_t len = strlen(buf); | ||
| + size_t len = strnlen(buf, buf_size); | ||
| if (len > 0 && buf[len - 1] == ',') { |
jdstrand
Feb 7, 2017
Contributor
strnlen() can return buf_size here which would indicate buf is a non-terminated string. Now, because of all the sc_string_append() usage, this shouldn't happen, but to future-proof the code I suggest adding:
if (len == buf_size) {
die(...);
} else if (len > 0 && buf[len - 1] == ',') {
...
zyga
Feb 7, 2017
Contributor
Thanks for the tip. I don't have the instinct developed for using strnlen (imagine I worked me entire life without knowing it exited). I'll definitely be on the lookout for this more.
zyga commentedFeb 3, 2017
•
Edited 1 time
-
zyga
Feb 3, 2017
This branch changes sc_mount_opt2str to use sc_string_append, sc_must_snprintf and strnlen
instead of the traditional and unchecked equivalents from the standard library.
This change now requires the caller to provide a buffer so all the code using this function
was patched to adapt. The buffer size stayed the same as the old hard-coded internal buffer.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com