| Message ID | 679ea7421f73ce41515aca549982233f88bcefef.1655336146.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Coverity fixes | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > On Windows, we open files we suspect might be scripts, read the first > two bytes, and see whether they indicate a hash-bang line. We do not > initialize the byte _after_ those two bytes, therefore `strcmp()` is > inappropriate here. > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > run-command.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) This has been with us since cc3b7a97 (Windows: Make 'git help -a' work., 2008-01-14) and apparently nobody made loud enough noises to make us aware since then. The fix is trivially correct, of course. Will queue. > > diff --git a/run-command.c b/run-command.c > index 14f17830f51..2ba38850b4d 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -154,7 +154,7 @@ int is_executable(const char *name) > n = read(fd, buf, 2); > if (n == 2) > /* look for a she-bang */ > - if (!strcmp(buf, "#!")) > + if (!memcmp(buf, "#!", 2)) > st.st_mode |= S_IXUSR; > close(fd); > }
Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > On Windows, we open files we suspect might be scripts, read the first > two bytes, and see whether they indicate a hash-bang line. We do not > initialize the byte _after_ those two bytes, therefore `strcmp()` is > inappropriate here. Hmm, but buf _is_ initialized fully? Line 149: char buf[3] = { 0 }; C99 §6.7.8 21: "If there are [...] fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration." > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > run-command.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/run-command.c b/run-command.c > index 14f17830f51..2ba38850b4d 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -154,7 +154,7 @@ int is_executable(const char *name) > n = read(fd, buf, 2); > if (n == 2) > /* look for a she-bang */ > - if (!strcmp(buf, "#!")) > + if (!memcmp(buf, "#!", 2)) > st.st_mode |= S_IXUSR; > close(fd); > }
René Scharfe <l.s.r@web.de> writes: > Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget: >> From: Johannes Schindelin <johannes.schindelin@gmx.de> >> >> On Windows, we open files we suspect might be scripts, read the first >> two bytes, and see whether they indicate a hash-bang line. We do not >> initialize the byte _after_ those two bytes, therefore `strcmp()` is >> inappropriate here. > > Hmm, but buf _is_ initialized fully? Line 149: > > char buf[3] = { 0 }; Ahh, yeah, that changes the landscape quite a bit. We explicitly ask to read 2 bytes and look at the buf[] when read says it read 2 bytes, so this is another false positive X-<. >> diff --git a/run-command.c b/run-command.c >> index 14f17830f51..2ba38850b4d 100644 >> --- a/run-command.c >> +++ b/run-command.c >> @@ -154,7 +154,7 @@ int is_executable(const char *name) >> n = read(fd, buf, 2); >> if (n == 2) >> /* look for a she-bang */ >> - if (!strcmp(buf, "#!")) >> + if (!memcmp(buf, "#!", 2)) >> st.st_mode |= S_IXUSR; >> close(fd); >> } We can update the variable to char buf[2]; to match the updated code, I guess. The fewer bytes we use on stack, the better ;-).
Junio C Hamano <gitster@pobox.com> writes: >> Hmm, but buf _is_ initialized fully? Line 149: >> >> char buf[3] = { 0 }; > > Ahh, yeah, that changes the landscape quite a bit. Just for reference, this piece of code has been correct ever since it was introduced to help.c by cc3b7a97 (Windows: Make 'git help -a' work., 2008-01-14). With a larger context, we can see buf[3] that is NUL filled, which receives 2 bytes by read(), and strcmp() does look semantically more correct than memcmp(), even though there probably shouldn't be any correctness or performance difference. So, strcmp -> memcmp in this case is a strict dis-improvement. I merged it already to 'next', but I haven't pushed the result out, so I'll redo 'next' before I push the result out. Thanks.
diff --git a/run-command.c b/run-command.c index 14f17830f51..2ba38850b4d 100644 --- a/run-command.c +++ b/run-command.c @@ -154,7 +154,7 @@ int is_executable(const char *name) n = read(fd, buf, 2); if (n == 2) /* look for a she-bang */ - if (!strcmp(buf, "#!")) + if (!memcmp(buf, "#!", 2)) st.st_mode |= S_IXUSR; close(fd); }