| Message ID | 1337087078-24056-2-git-send-email-jim@meyering.net |
|---|---|
| State | New |
| Headers | show |
On Tue, May 15, 2012 at 1:04 PM, <jim@meyering.net> wrote: > From: Jim Meyering <meyering@redhat.com> > > Without this, envlist_to_environ may silently fail to copy all > strings into the destination buffer, and both callers would leak > any env strings allocated after a failing strdup, because the > freeing code stops at the first NULL pointer. > > Signed-off-by: Jim Meyering <meyering@redhat.com> > --- > envlist.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/envlist.c b/envlist.c > index f2303cd..2bbd99c 100644 > --- a/envlist.c > +++ b/envlist.c > @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count) > > for (entry = envlist->el_entries.lh_first; entry != NULL; > entry = entry->ev_link.le_next) { > - *(penv++) = strdup(entry->ev_var); > + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { > + char **e = env; > + while (e != penv) { > + free(*e++); > + } > + free(env); > + return NULL; > + } ERROR: code indent should never use tabs #82: FILE: envlist.c:238: +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$ ERROR: do not use assignment in if condition #82: FILE: envlist.c:238: + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { ERROR: code indent should never use tabs #83: FILE: envlist.c:239: +^I^I^Ichar **e = env;$ ERROR: code indent should never use tabs #84: FILE: envlist.c:240: +^I^I^Iwhile (e != penv) {$ ERROR: code indent should never use tabs #85: FILE: envlist.c:241: +^I^I^I^Ifree(*e++);$ ERROR: code indent should never use tabs #86: FILE: envlist.c:242: +^I^I^I}$ ERROR: code indent should never use tabs #87: FILE: envlist.c:243: +^I^I^Ifree(env);$ ERROR: code indent should never use tabs #88: FILE: envlist.c:244: +^I^I^Ireturn NULL;$ ERROR: code indent should never use tabs #89: FILE: envlist.c:245: +^I^I}$ total: 9 errors, 0 warnings, 15 lines checked Please fix. > } > *penv = NULL; /* NULL terminate the list */ > > -- > 1.7.10.2.484.gcd07cc5 > >
Blue Swirl wrote: > On Tue, May 15, 2012 at 1:04 PM, <jim@meyering.net> wrote: >> From: Jim Meyering <meyering@redhat.com> >> >> Without this, envlist_to_environ may silently fail to copy all >> strings into the destination buffer, and both callers would leak >> any env strings allocated after a failing strdup, because the >> freeing code stops at the first NULL pointer. >> >> Signed-off-by: Jim Meyering <meyering@redhat.com> >> --- >> envlist.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/envlist.c b/envlist.c >> index f2303cd..2bbd99c 100644 >> --- a/envlist.c >> +++ b/envlist.c >> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count) >> >> for (entry = envlist->el_entries.lh_first; entry != NULL; >> entry = entry->ev_link.le_next) { >> - *(penv++) = strdup(entry->ev_var); >> + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { >> + char **e = env; >> + while (e != penv) { >> + free(*e++); >> + } >> + free(env); >> + return NULL; >> + } > > ERROR: code indent should never use tabs > #82: FILE: envlist.c:238: > +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$ That entire file is indented solely with TABs, so adding these new lines using spaces for indentation seems unjustified: the mix tends to make the code unreadable in some contexts (email quoting, for one). How about two patches: one to convert all leading TABs in envlist.c to spaces, and the next to make the above change, but indenting with spaces? > ERROR: do not use assignment in if condition > #82: FILE: envlist.c:238: > + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { I agree with the sentiment, but found that the alternative was less readable and less maintainable, since I'd have to increment "penv" in two places (both in the if-block and after it) rather than in just one. However, I've just realized I can hoist the "penv++" increment into the "for-statement", in which case it's ok: for (entry = envlist->el_entries.lh_first; entry != NULL; entry = entry->ev_link.le_next, penv++) { *penv = strdup(entry->ev_var); if (*penv == NULL) { char **e = env; while (e <= penv) { free(*e++); } free(env); return NULL; } } Your move. Which would you prefer? 1) two patches: one replacing all leading TABs with equivalent spaces, then the above patch 2) one patch, indented using TABs, in spite of the checkpatch failure 3) one patch, indented using spaces, in spite of the consistency issue ... > total: 9 errors, 0 warnings, 15 lines checked > > Please fix.
On Mon, May 21, 2012 at 10:25 AM, Jim Meyering <jim@meyering.net> wrote: > Blue Swirl wrote: >> On Tue, May 15, 2012 at 1:04 PM, <jim@meyering.net> wrote: >>> From: Jim Meyering <meyering@redhat.com> >>> >>> Without this, envlist_to_environ may silently fail to copy all >>> strings into the destination buffer, and both callers would leak >>> any env strings allocated after a failing strdup, because the >>> freeing code stops at the first NULL pointer. >>> >>> Signed-off-by: Jim Meyering <meyering@redhat.com> >>> --- >>> envlist.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/envlist.c b/envlist.c >>> index f2303cd..2bbd99c 100644 >>> --- a/envlist.c >>> +++ b/envlist.c >>> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count) >>> >>> for (entry = envlist->el_entries.lh_first; entry != NULL; >>> entry = entry->ev_link.le_next) { >>> - *(penv++) = strdup(entry->ev_var); >>> + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { >>> + char **e = env; >>> + while (e != penv) { >>> + free(*e++); >>> + } >>> + free(env); >>> + return NULL; >>> + } >> >> ERROR: code indent should never use tabs >> #82: FILE: envlist.c:238: >> +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$ > > That entire file is indented solely with TABs, so adding these new > lines using spaces for indentation seems unjustified: the mix tends > to make the code unreadable in some contexts (email quoting, for one). > How about two patches: one to convert all leading TABs in envlist.c to > spaces, and the next to make the above change, but indenting with spaces? > >> ERROR: do not use assignment in if condition >> #82: FILE: envlist.c:238: >> + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { > > I agree with the sentiment, but found that the alternative was less > readable and less maintainable, since I'd have to increment "penv" in > two places (both in the if-block and after it) rather than in just one. > However, I've just realized I can hoist the "penv++" increment into the > "for-statement", in which case it's ok: > > for (entry = envlist->el_entries.lh_first; entry != NULL; > entry = entry->ev_link.le_next, penv++) { > *penv = strdup(entry->ev_var); > if (*penv == NULL) { > char **e = env; > while (e <= penv) { > free(*e++); > } > free(env); > return NULL; > } > } > > Your move. Which would you prefer? > 1) two patches: one replacing all leading TABs with equivalent spaces, > then the above patch > 2) one patch, indented using TABs, in spite of the checkpatch failure > 3) one patch, indented using spaces, in spite of the consistency issue 1) (or 3). Though for v1.1, maybe 3) is the smaller fix and later do 1) for 1.2. > > ... >> total: 9 errors, 0 warnings, 15 lines checked >> >> Please fix.
Am 21.05.2012 19:56, schrieb Blue Swirl: > On Mon, May 21, 2012 at 10:25 AM, Jim Meyering <jim@meyering.net> wrote: >> Blue Swirl wrote: >>> On Tue, May 15, 2012 at 1:04 PM, <jim@meyering.net> wrote: >>>> From: Jim Meyering <meyering@redhat.com> >>>> >>>> Without this, envlist_to_environ may silently fail to copy all >>>> strings into the destination buffer, and both callers would leak >>>> any env strings allocated after a failing strdup, because the >>>> freeing code stops at the first NULL pointer. >>>> >>>> Signed-off-by: Jim Meyering <meyering@redhat.com> >>>> --- >>>> envlist.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/envlist.c b/envlist.c >>>> index f2303cd..2bbd99c 100644 >>>> --- a/envlist.c >>>> +++ b/envlist.c >>>> @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count) >>>> >>>> for (entry = envlist->el_entries.lh_first; entry != NULL; >>>> entry = entry->ev_link.le_next) { >>>> - *(penv++) = strdup(entry->ev_var); >>>> + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { >>>> + char **e = env; >>>> + while (e != penv) { >>>> + free(*e++); >>>> + } >>>> + free(env); >>>> + return NULL; >>>> + } >>> >>> ERROR: code indent should never use tabs >>> #82: FILE: envlist.c:238: >>> +^I^Iif ((*(penv++) = strdup(entry->ev_var)) == NULL) {$ >> >> That entire file is indented solely with TABs, so adding these new >> lines using spaces for indentation seems unjustified: the mix tends >> to make the code unreadable in some contexts (email quoting, for one). >> How about two patches: one to convert all leading TABs in envlist.c to >> spaces, and the next to make the above change, but indenting with spaces? >> >>> ERROR: do not use assignment in if condition >>> #82: FILE: envlist.c:238: >>> + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { >> >> I agree with the sentiment, but found that the alternative was less >> readable and less maintainable, since I'd have to increment "penv" in >> two places (both in the if-block and after it) rather than in just one. >> However, I've just realized I can hoist the "penv++" increment into the >> "for-statement", in which case it's ok: >> >> for (entry = envlist->el_entries.lh_first; entry != NULL; >> entry = entry->ev_link.le_next, penv++) { >> *penv = strdup(entry->ev_var); >> if (*penv == NULL) { >> char **e = env; >> while (e <= penv) { >> free(*e++); >> } >> free(env); >> return NULL; >> } >> } >> >> Your move. Which would you prefer? >> 1) two patches: one replacing all leading TABs with equivalent spaces, >> then the above patch >> 2) one patch, indented using TABs, in spite of the checkpatch failure >> 3) one patch, indented using spaces, in spite of the consistency issue > > 1) (or 3). Though for v1.1, maybe 3) is the smaller fix and later do 1) for 1.2. A patch replacing tabs by spaces isn't really the kind of patches that we would want to avoid during freeze. It's easy enough to check with git diff -w that it doesn't change anything semantically. Kevin
Kevin Wolf wrote: > A patch replacing tabs by spaces isn't really the kind of patches that > we would want to avoid during freeze. It's easy enough to check with git > diff -w that it doesn't change anything semantically. That makes sense, so I've posted two patches: 1) two patches: one replacing all leading TABs with equivalent spaces, then the above patch Note however, that envlist.c must predate checkpatch.pl, since that patch provokes numerous warnings about style issues: (hmm... I've just noticed these first two, which seem to suggest that even for comments I should remove the TABs. Is it worth a V2 to fix those two lines? Or are they false positives, since they're not really "code" indentation? ) ERROR: code indent should never use tabs #23: FILE: envlist.c:11: + const char *ev_var;^I^I^I/* actual env value */$ ERROR: code indent should never use tabs #30: FILE: envlist.c:16: + QLIST_HEAD(, envlist_entry) el_entries;^I/* actual entries */$ ERROR: code indent should never use tabs #31: FILE: envlist.c:17: + size_t el_count;^I^I^I/* number of entries */$ WARNING: space prohibited between function name and open parenthesis '(' #44: FILE: envlist.c:32: + if ((envlist = malloc(sizeof (*envlist))) == NULL) ERROR: do not use assignment in if condition #44: FILE: envlist.c:32: + if ((envlist = malloc(sizeof (*envlist))) == NULL) WARNING: braces {} are necessary for all arms of this statement #44: FILE: envlist.c:32: + if ((envlist = malloc(sizeof (*envlist))) == NULL) [...] ERROR: return is not a function, parentheses are not required #45: FILE: envlist.c:33: + return (NULL); ERROR: return is not a function, parentheses are not required #53: FILE: envlist.c:38: + return (envlist); ERROR: return is not a function, parentheses are not required #90: FILE: envlist.c:75: + return (envlist_parse(envlist, env, &envlist_setenv)); ERROR: return is not a function, parentheses are not required #99: FILE: envlist.c:87: + return (envlist_parse(envlist, env, &envlist_unsetenv)); WARNING: braces {} are necessary for all arms of this statement #138: FILE: envlist.c:105: + if ((envlist == NULL) || (env == NULL)) [...] ERROR: return is not a function, parentheses are not required #139: FILE: envlist.c:106: + return (EINVAL); ERROR: do not use assignment in if condition #145: FILE: envlist.c:112: + if ((tmpenv = strdup(env)) == NULL) WARNING: braces {} are necessary for all arms of this statement #145: FILE: envlist.c:112: + if ((tmpenv = strdup(env)) == NULL) [...] ERROR: return is not a function, parentheses are not required #146: FILE: envlist.c:113: + return (errno); ERROR: return is not a function, parentheses are not required #152: FILE: envlist.c:119: + return (errno); ERROR: return is not a function, parentheses are not required #158: FILE: envlist.c:125: + return (0); WARNING: braces {} are necessary for all arms of this statement #210: FILE: envlist.c:141: + if ((envlist == NULL) || (env == NULL)) [...] ERROR: return is not a function, parentheses are not required #211: FILE: envlist.c:142: + return (EINVAL); ERROR: do not use assignment in if condition #214: FILE: envlist.c:145: + if ((eq_sign = strchr(env, '=')) == NULL) WARNING: braces {} are necessary for all arms of this statement #214: FILE: envlist.c:145: + if ((eq_sign = strchr(env, '=')) == NULL) [...] ERROR: return is not a function, parentheses are not required #215: FILE: envlist.c:146: + return (EINVAL); WARNING: braces {} are necessary for all arms of this statement #225: FILE: envlist.c:156: + if (strncmp(entry->ev_var, env, envname_len) == 0) [...] WARNING: space prohibited between function name and open parenthesis '(' #237: FILE: envlist.c:168: + if ((entry = malloc(sizeof (*entry))) == NULL) ERROR: do not use assignment in if condition #237: FILE: envlist.c:168: + if ((entry = malloc(sizeof (*entry))) == NULL) WARNING: braces {} are necessary for all arms of this statement #237: FILE: envlist.c:168: + if ((entry = malloc(sizeof (*entry))) == NULL) [...] ERROR: return is not a function, parentheses are not required #238: FILE: envlist.c:169: + return (errno); ERROR: do not use assignment in if condition #239: FILE: envlist.c:170: + if ((entry->ev_var = strdup(env)) == NULL) { ERROR: return is not a function, parentheses are not required #241: FILE: envlist.c:172: + return (errno); ERROR: return is not a function, parentheses are not required #245: FILE: envlist.c:176: + return (0); WARNING: braces {} are necessary for all arms of this statement #284: FILE: envlist.c:189: + if ((envlist == NULL) || (env == NULL)) [...] ERROR: return is not a function, parentheses are not required #285: FILE: envlist.c:190: + return (EINVAL); WARNING: braces {} are necessary for all arms of this statement #288: FILE: envlist.c:193: + if (strchr(env, '=') != NULL) [...] ERROR: return is not a function, parentheses are not required #289: FILE: envlist.c:194: + return (EINVAL); WARNING: braces {} are necessary for all arms of this statement #298: FILE: envlist.c:203: + if (strncmp(entry->ev_var, env, envname_len) == 0) [...] ERROR: return is not a function, parentheses are not required #308: FILE: envlist.c:213: + return (0); WARNING: space prohibited between function name and open parenthesis '(' #324: FILE: envlist.c:232: + penv = env = malloc((envlist->el_count + 1) * sizeof (char *)); WARNING: braces {} are necessary for all arms of this statement #325: FILE: envlist.c:233: + if (env == NULL) [...] ERROR: return is not a function, parentheses are not required #326: FILE: envlist.c:234: + return (NULL); WARNING: braces {} are necessary for all arms of this statement #341: FILE: envlist.c:242: + if (count != NULL) [...] ERROR: return is not a function, parentheses are not required #345: FILE: envlist.c:245: + return (env); total: 26 errors, 15 warnings, 321 lines checked .qe-strdup/0001-envlist.c-convert-many-leading-TABs-to-spaces-via-ex.patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. total: 0 errors, 0 warnings, 18 lines checked .qe-strdup/0002-envlist.c-handle-strdup-failure.patch has no obvious style problems and is ready for submission.
Am 22.05.2012 11:05, schrieb Jim Meyering: > Kevin Wolf wrote: >> A patch replacing tabs by spaces isn't really the kind of patches that >> we would want to avoid during freeze. It's easy enough to check with git >> diff -w that it doesn't change anything semantically. > > That makes sense, so I've posted two patches: > > 1) two patches: one replacing all leading TABs with equivalent spaces, > then the above patch > > Note however, that envlist.c must predate checkpatch.pl, since > that patch provokes numerous warnings about style issues: Quite possible. Most files in qemu will still contain some violations. > (hmm... I've just noticed these first two, which seem to suggest > that even for comments I should remove the TABs. Is it worth > a V2 to fix those two lines? Or are they false positives, since > they're not really "code" indentation? ) We don't use tabs at all where possible. There shouldn't be much more exceptions than the Makefiles. Kevin
Kevin Wolf wrote: > Am 22.05.2012 11:05, schrieb Jim Meyering: >> Kevin Wolf wrote: >>> A patch replacing tabs by spaces isn't really the kind of patches that >>> we would want to avoid during freeze. It's easy enough to check with git >>> diff -w that it doesn't change anything semantically. >> >> That makes sense, so I've posted two patches: >> >> 1) two patches: one replacing all leading TABs with equivalent spaces, >> then the above patch >> >> Note however, that envlist.c must predate checkpatch.pl, since >> that patch provokes numerous warnings about style issues: > > Quite possible. Most files in qemu will still contain some violations. > >> (hmm... I've just noticed these first two, which seem to suggest >> that even for comments I should remove the TABs. Is it worth >> a V2 to fix those two lines? Or are they false positives, since >> they're not really "code" indentation? ) > > We don't use tabs at all where possible. There shouldn't be much more > exceptions than the Makefiles. Sounds like a v2 would be welcome. Here you go...
diff --git a/envlist.c b/envlist.c index f2303cd..2bbd99c 100644 --- a/envlist.c +++ b/envlist.c @@ -235,7 +235,14 @@ envlist_to_environ(const envlist_t *envlist, size_t *count) for (entry = envlist->el_entries.lh_first; entry != NULL; entry = entry->ev_link.le_next) { - *(penv++) = strdup(entry->ev_var); + if ((*(penv++) = strdup(entry->ev_var)) == NULL) { + char **e = env; + while (e != penv) { + free(*e++); + } + free(env); + return NULL; + } } *penv = NULL; /* NULL terminate the list */