* [PATCH] avoid NULL dereference on failed malloc @ 2009-06-14 19:46 Jim Meyering 2009-06-15 7:49 ` Michael J Gruber 0 siblings, 1 reply; 9+ messages in thread From: Jim Meyering @ 2009-06-14 19:46 UTC (permalink / raw) To: git list * builtin-remote.c (get_one_entry): Use xmalloc, not malloc. Signed-off-by: Jim Meyering <meyering@redhat.com> --- builtin-remote.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin-remote.c b/builtin-remote.c index 709f8a6..406fb85 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -1282,7 +1282,7 @@ static int get_one_entry(struct remote *remote, void *priv) if (remote->url_nr > 0) { utilp = &(string_list_append(remote->name, list)->util); - *utilp = malloc(strlen(remote->url[0])+strlen(" (fetch)")+1); + *utilp = xmalloc(strlen(remote->url[0])+strlen(" (fetch)")+1); strcpy((char *) *utilp, remote->url[0]); strcat((char *) *utilp, " (fetch)"); } else @@ -1297,7 +1297,7 @@ static int get_one_entry(struct remote *remote, void *priv) for (i = 0; i < url_nr; i++) { utilp = &(string_list_append(remote->name, list)->util); - *utilp = malloc(strlen(url[i])+strlen(" (push)")+1); + *utilp = xmalloc(strlen(url[i])+strlen(" (push)")+1); strcpy((char *) *utilp, url[i]); strcat((char *) *utilp, " (push)"); } -- 1.6.3.2.406.gd6a466 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] avoid NULL dereference on failed malloc 2009-06-14 19:46 [PATCH] avoid NULL dereference on failed malloc Jim Meyering @ 2009-06-15 7:49 ` Michael J Gruber 2009-06-15 20:45 ` [PATCH] builtin-remote: (get_one_entry): use strbuf Bert Wesarg 0 siblings, 1 reply; 9+ messages in thread From: Michael J Gruber @ 2009-06-15 7:49 UTC (permalink / raw) To: Jim Meyering; +Cc: git list, Junio C Hamano Jim Meyering venit, vidit, dixit 14.06.2009 21:46: > > * builtin-remote.c (get_one_entry): Use xmalloc, not malloc. Learning something new with every patch... Sorry, Junio; thanks, Jim! Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] builtin-remote: (get_one_entry): use strbuf 2009-06-15 7:49 ` Michael J Gruber @ 2009-06-15 20:45 ` Bert Wesarg 2009-06-16 7:39 ` Michael J Gruber 0 siblings, 1 reply; 9+ messages in thread From: Bert Wesarg @ 2009-06-15 20:45 UTC (permalink / raw) To: Michael J Gruber; +Cc: Jim Meyering, git, Junio C Hamano, Bert Wesarg Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> --- On Mon, Jun 15, 2009 at 09:49, Michael J Gruber<git@drmicha.warpmail.net> wrote: > Jim Meyering venit, vidit, dixit 14.06.2009 21:46: >> >> * builtin-remote.c (get_one_entry): Use xmalloc, not malloc. > > Learning something new with every patch... Sorry, Junio; thanks, Jim! > One more reason to re-use existing string handling functions. Regards, Bert builtin-remote.c | 21 ++++++++++----------- 1 files changed, 10 insertions(+), 11 deletions(-) diff --git a/builtin-remote.c b/builtin-remote.c index 709f8a6..31adeaa 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -1276,17 +1276,17 @@ static int update(int argc, const char **argv) static int get_one_entry(struct remote *remote, void *priv) { struct string_list *list = priv; + struct strbuf url_buf = STRBUF_INIT; + const char *url_str = NULL; const char **url; int i, url_nr; - void **utilp; if (remote->url_nr > 0) { - utilp = &(string_list_append(remote->name, list)->util); - *utilp = xmalloc(strlen(remote->url[0])+strlen(" (fetch)")+1); - strcpy((char *) *utilp, remote->url[0]); - strcat((char *) *utilp, " (fetch)"); - } else - string_list_append(remote->name, list)->util = NULL; + strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]); + url_str = strbuf_detach(&url_buf, NULL); + } + string_list_append(remote->name, list)->util = url_str; + if (remote->pushurl_nr) { url = remote->pushurl; url_nr = remote->pushurl_nr; @@ -1296,10 +1296,9 @@ static int get_one_entry(struct remote *remote, void *priv) } for (i = 0; i < url_nr; i++) { - utilp = &(string_list_append(remote->name, list)->util); - *utilp = xmalloc(strlen(url[i])+strlen(" (push)")+1); - strcpy((char *) *utilp, url[i]); - strcat((char *) *utilp, " (push)"); + strbuf_addf(&url_buf, "%s (push)", url[i]); + url_str = strbuf_detach(&url_buf, NULL); + string_list_append(remote->name, list)->util = url_str; } return 0; -- tg: (d6a466e..) bw/remote-v-use-strbuf (depends on: next) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] builtin-remote: (get_one_entry): use strbuf 2009-06-15 20:45 ` [PATCH] builtin-remote: (get_one_entry): use strbuf Bert Wesarg @ 2009-06-16 7:39 ` Michael J Gruber 2009-06-16 7:56 ` Bert Wesarg 0 siblings, 1 reply; 9+ messages in thread From: Michael J Gruber @ 2009-06-16 7:39 UTC (permalink / raw) To: Bert Wesarg; +Cc: Jim Meyering, git, Junio C Hamano Bert Wesarg venit, vidit, dixit 15.06.2009 22:45: > Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> > > --- > > On Mon, Jun 15, 2009 at 09:49, Michael J Gruber<git@drmicha.warpmail.net> wrote: >> Jim Meyering venit, vidit, dixit 14.06.2009 21:46: >>> >>> * builtin-remote.c (get_one_entry): Use xmalloc, not malloc. >> >> Learning something new with every patch... Sorry, Junio; thanks, Jim! >> > One more reason to re-use existing string handling functions. Well, when we discussed this before v2 I asked for guidance about strbuf, esp. regarding the issue of allocating/freeing. From your patch I infer that "strbuf_detach" is what I was looking for. (And yes, it is in the api doc where I overlooked it.) > builtin-remote.c | 21 ++++++++++----------- > 1 files changed, 10 insertions(+), 11 deletions(-) > > diff --git a/builtin-remote.c b/builtin-remote.c > index 709f8a6..31adeaa 100644 > --- a/builtin-remote.c > +++ b/builtin-remote.c For whatever reason, your patch does not apply (am) here on top of next + Jim's patch. Given the context (xmallocs), it looks like it's against something + Jim's patch. OTOH: 709f8a6 show's a get_one_entry with mallocs. Did you hand edit the diff? Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] builtin-remote: (get_one_entry): use strbuf 2009-06-16 7:39 ` Michael J Gruber @ 2009-06-16 7:56 ` Bert Wesarg 2009-06-16 10:49 ` Michael J Gruber 0 siblings, 1 reply; 9+ messages in thread From: Bert Wesarg @ 2009-06-16 7:56 UTC (permalink / raw) To: Michael J Gruber; +Cc: Jim Meyering, git, Junio C Hamano On Tue, Jun 16, 2009 at 09:39, Michael J Gruber<git@drmicha.warpmail.net> wrote: > Bert Wesarg venit, vidit, dixit 15.06.2009 22:45: >> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> >> >> --- >> >> On Mon, Jun 15, 2009 at 09:49, Michael J Gruber<git@drmicha.warpmail.net> wrote: >>> Jim Meyering venit, vidit, dixit 14.06.2009 21:46: >>>> >>>> * builtin-remote.c (get_one_entry): Use xmalloc, not malloc. >>> >>> Learning something new with every patch... Sorry, Junio; thanks, Jim! >>> >> One more reason to re-use existing string handling functions. > > Well, when we discussed this before v2 I asked for guidance about > strbuf, esp. regarding the issue of allocating/freeing. Well, I stopped reading after this question of yours: "But what do strbufs bring us here?" > From your patch > I infer that "strbuf_detach" is what I was looking for. (And yes, it is > in the api doc where I overlooked it.) > >> builtin-remote.c | 21 ++++++++++----------- >> 1 files changed, 10 insertions(+), 11 deletions(-) >> >> diff --git a/builtin-remote.c b/builtin-remote.c >> index 709f8a6..31adeaa 100644 >> --- a/builtin-remote.c >> +++ b/builtin-remote.c > > For whatever reason, your patch does not apply (am) here on top of next > + Jim's patch. Given the context (xmallocs), it looks like it's against > something + Jim's patch. OTOH: 709f8a6 show's a get_one_entry with > mallocs. Did you hand edit the diff? Its on top of next (d6a466e528119011d512379f7f9dfac26deb7fd9), plus hand editing s/malloc/xmalloc/g. Sorry for this. Bert > > Michael > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] builtin-remote: (get_one_entry): use strbuf 2009-06-16 7:56 ` Bert Wesarg @ 2009-06-16 10:49 ` Michael J Gruber 2009-06-22 8:24 ` Bert Wesarg 0 siblings, 1 reply; 9+ messages in thread From: Michael J Gruber @ 2009-06-16 10:49 UTC (permalink / raw) To: Bert Wesarg; +Cc: Jim Meyering, git, Junio C Hamano Bert Wesarg venit, vidit, dixit 16.06.2009 09:56: > On Tue, Jun 16, 2009 at 09:39, Michael J Gruber<git@drmicha.warpmail.net> wrote: >> Bert Wesarg venit, vidit, dixit 15.06.2009 22:45: >>> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> >>> >>> --- >>> >>> On Mon, Jun 15, 2009 at 09:49, Michael J Gruber<git@drmicha.warpmail.net> wrote: >>>> Jim Meyering venit, vidit, dixit 14.06.2009 21:46: >>>>> >>>>> * builtin-remote.c (get_one_entry): Use xmalloc, not malloc. >>>> >>>> Learning something new with every patch... Sorry, Junio; thanks, Jim! >>>> >>> One more reason to re-use existing string handling functions. >> >> Well, when we discussed this before v2 I asked for guidance about >> strbuf, esp. regarding the issue of allocating/freeing. > Well, I stopped reading after this question of yours: "But what do > strbufs bring us here?" Well, as I said I was strbuf-agnostic - some questions are really meant to be questions ;) In any case, I've learned from your patch. > >> From your patch >> I infer that "strbuf_detach" is what I was looking for. (And yes, it is >> in the api doc where I overlooked it.) >> >>> builtin-remote.c | 21 ++++++++++----------- >>> 1 files changed, 10 insertions(+), 11 deletions(-) >>> >>> diff --git a/builtin-remote.c b/builtin-remote.c >>> index 709f8a6..31adeaa 100644 >>> --- a/builtin-remote.c >>> +++ b/builtin-remote.c >> >> For whatever reason, your patch does not apply (am) here on top of next >> + Jim's patch. Given the context (xmallocs), it looks like it's against >> something + Jim's patch. OTOH: 709f8a6 show's a get_one_entry with >> mallocs. Did you hand edit the diff? > Its on top of next (d6a466e528119011d512379f7f9dfac26deb7fd9), plus > hand editing s/malloc/xmalloc/g. > Sorry for this. > > Bert Junio will have to deal with it... Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] builtin-remote: (get_one_entry): use strbuf 2009-06-16 10:49 ` Michael J Gruber @ 2009-06-22 8:24 ` Bert Wesarg 2009-06-22 21:32 ` Jeff King 0 siblings, 1 reply; 9+ messages in thread From: Bert Wesarg @ 2009-06-22 8:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: Michael J Gruber, Jim Meyering, git, Bert Wesarg Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> --- On Tue, Jun 16, 2009 at 12:49, Michael J Gruber<git@drmicha.warpmail.net> wrote: > Bert Wesarg venit, vidit, dixit 16.06.2009 09:56: >> On Tue, Jun 16, 2009 at 09:39, Michael J Gruber<git@drmicha.warpmail.net> wrote: >>> For whatever reason, your patch does not apply (am) here on top of next >>> + Jim's patch. Given the context (xmallocs), it looks like it's against >>> something + Jim's patch. OTOH: 709f8a6 show's a get_one_entry with >>> mallocs. Did you hand edit the diff? >> Its on top of next (d6a466e528119011d512379f7f9dfac26deb7fd9), plus >> hand editing s/malloc/xmalloc/g. >> Sorry for this. >> >> Bert > > Junio will have to deal with it... Here is an updated version. Bert builtin-remote.c | 21 ++++++++++----------- 1 files changed, 10 insertions(+), 11 deletions(-) diff --git a/builtin-remote.c b/builtin-remote.c index 658d578..d47a124 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -1276,17 +1276,17 @@ static int update(int argc, const char **argv) static int get_one_entry(struct remote *remote, void *priv) { struct string_list *list = priv; + struct strbuf url_buf = STRBUF_INIT; + const char *url_str = NULL; const char **url; int i, url_nr; - void **utilp; if (remote->url_nr > 0) { - utilp = &(string_list_append(remote->name, list)->util); - *utilp = xmalloc(strlen(remote->url[0])+strlen(" (fetch)")+1); - strcpy((char *) *utilp, remote->url[0]); - strcat((char *) *utilp, " (fetch)"); - } else - string_list_append(remote->name, list)->util = NULL; + strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]); + url_str = strbuf_detach(&url_buf, NULL); + } + string_list_append(remote->name, list)->util = url_str; + if (remote->pushurl_nr) { url = remote->pushurl; url_nr = remote->pushurl_nr; @@ -1296,10 +1296,9 @@ static int get_one_entry(struct remote *remote, void *priv) } for (i = 0; i < url_nr; i++) { - utilp = &(string_list_append(remote->name, list)->util); - *utilp = xmalloc(strlen(url[i])+strlen(" (push)")+1); - strcpy((char *) *utilp, url[i]); - strcat((char *) *utilp, " (push)"); + strbuf_addf(&url_buf, "%s (push)", url[i]); + url_str = strbuf_detach(&url_buf, NULL); + string_list_append(remote->name, list)->util = url_str; } return 0; -- tg: (363bdbe..) bw/remote-v-use-strbuf (depends on: next) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] builtin-remote: (get_one_entry): use strbuf 2009-06-22 8:24 ` Bert Wesarg @ 2009-06-22 21:32 ` Jeff King 2009-06-22 22:27 ` [PATCH v2] " Bert Wesarg 0 siblings, 1 reply; 9+ messages in thread From: Jeff King @ 2009-06-22 21:32 UTC (permalink / raw) To: Bert Wesarg; +Cc: Junio C Hamano, Michael J Gruber, Jim Meyering, git On Mon, Jun 22, 2009 at 10:24:19AM +0200, Bert Wesarg wrote: > --- a/builtin-remote.c > +++ b/builtin-remote.c > @@ -1276,17 +1276,17 @@ static int update(int argc, const char **argv) > static int get_one_entry(struct remote *remote, void *priv) > { > struct string_list *list = priv; > + struct strbuf url_buf = STRBUF_INIT; > + const char *url_str = NULL; > const char **url; > int i, url_nr; > - void **utilp; > > if (remote->url_nr > 0) { > - utilp = &(string_list_append(remote->name, list)->util); > - *utilp = xmalloc(strlen(remote->url[0])+strlen(" (fetch)")+1); > - strcpy((char *) *utilp, remote->url[0]); > - strcat((char *) *utilp, " (fetch)"); > - } else > - string_list_append(remote->name, list)->util = NULL; > + strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]); > + url_str = strbuf_detach(&url_buf, NULL); > + } > + string_list_append(remote->name, list)->util = url_str; > + This causes const warnings due to the 'const' on url_str. One solution is just s/const char/char/. However, I think it is actually more readable to do away with url_str entirely. The original if/else logic was much easier to follow than realizing that url_str is initialized to NULL, and then never changed. IOW: if (remote->url_nr > 0) { strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]); string_list_append(remote->name, list)->util = strbuf_detach(&url_buf, NULL); } else string_list_append(remote->name, list)->util = NULL; > @@ -1296,10 +1296,9 @@ static int get_one_entry(struct remote *remote, void *priv) > } > for (i = 0; i < url_nr; i++) > { > - utilp = &(string_list_append(remote->name, list)->util); > - *utilp = xmalloc(strlen(url[i])+strlen(" (push)")+1); > - strcpy((char *) *utilp, url[i]); > - strcat((char *) *utilp, " (push)"); > + strbuf_addf(&url_buf, "%s (push)", url[i]); > + url_str = strbuf_detach(&url_buf, NULL); > + string_list_append(remote->name, list)->util = url_str; > } And then this one is re-using url_str for a totally unrelated thing, but could just be: string_list_append(remote->name, list)->util = strbuf_detach(&url_buf, NULL); But that is somewhat nit-picking. As long as the const-warning goes away, I will be happy enough. -Peff ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] builtin-remote: (get_one_entry): use strbuf 2009-06-22 21:32 ` Jeff King @ 2009-06-22 22:27 ` Bert Wesarg 0 siblings, 0 replies; 9+ messages in thread From: Bert Wesarg @ 2009-06-22 22:27 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Michael J Gruber, Jim Meyering, git, Bert Wesarg Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com> --- v2: - remove assigment indirection - keep old code flow On Mon, Jun 22, 2009 at 23:32, Jeff King <peff@peff.net> wrote: > But that is somewhat nit-picking. As long as the const-warning goes > away, I will be happy enough. > Thanks for the review. I have no objections to your comments and have all incroporated into v2. Thanks, Bert > -Peff builtin-remote.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-) diff --git a/builtin-remote.c b/builtin-remote.c index 658d578..2fb76d3 100644 --- a/builtin-remote.c +++ b/builtin-remote.c @@ -1276,15 +1276,14 @@ static int update(int argc, const char **argv) static int get_one_entry(struct remote *remote, void *priv) { struct string_list *list = priv; + struct strbuf url_buf = STRBUF_INIT; const char **url; int i, url_nr; - void **utilp; if (remote->url_nr > 0) { - utilp = &(string_list_append(remote->name, list)->util); - *utilp = xmalloc(strlen(remote->url[0])+strlen(" (fetch)")+1); - strcpy((char *) *utilp, remote->url[0]); - strcat((char *) *utilp, " (fetch)"); + strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]); + string_list_append(remote->name, list)->util = + strbuf_detach(&url_buf, NULL); } else string_list_append(remote->name, list)->util = NULL; if (remote->pushurl_nr) { @@ -1296,10 +1295,9 @@ static int get_one_entry(struct remote *remote, void *priv) } for (i = 0; i < url_nr; i++) { - utilp = &(string_list_append(remote->name, list)->util); - *utilp = xmalloc(strlen(url[i])+strlen(" (push)")+1); - strcpy((char *) *utilp, url[i]); - strcat((char *) *utilp, " (push)"); + strbuf_addf(&url_buf, "%s (push)", url[i]); + string_list_append(remote->name, list)->util = + strbuf_detach(&url_buf, NULL); } return 0; -- tg: (d4b46b0..) bw/remote-v-use-strbuf (depends on: next) ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-06-22 22:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-06-14 19:46 [PATCH] avoid NULL dereference on failed malloc Jim Meyering 2009-06-15 7:49 ` Michael J Gruber 2009-06-15 20:45 ` [PATCH] builtin-remote: (get_one_entry): use strbuf Bert Wesarg 2009-06-16 7:39 ` Michael J Gruber 2009-06-16 7:56 ` Bert Wesarg 2009-06-16 10:49 ` Michael J Gruber 2009-06-22 8:24 ` Bert Wesarg 2009-06-22 21:32 ` Jeff King 2009-06-22 22:27 ` [PATCH v2] " Bert Wesarg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).