threads / patch / 19810

patchavoid NULL dereference on failed malloc

Subject: [PATCH] avoid NULL dereference on failed malloc

## tl;dr

9 messages between Jun 14, 2009 and Jun 22, 2009. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Jim Meyering· Jun 14, 2009, 19:46 UTC · lore
* 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(-)
Show changes to builtin-remote.c +2 −3
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
Michael J Gruber· Jun 15, 2009, 07:49 UTC · re: Jim Meyering · lore

Re: [PATCH] avoid NULL dereference on failed malloc

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
Bert Wesarg· Jun 15, 2009, 20:45 UTC · re: Michael J Gruber · lore

[PATCH] builtin-remote: (get_one_entry): use strbuf

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:
Show 6 quoted lines
> 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(-)
Show changes to builtin-remote.c +10 −11
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)
Michael J Gruber· Jun 16, 2009, 07:39 UTC · re: Bert Wesarg · lore

Re: [PATCH] builtin-remote: (get_one_entry): use strbuf

Bert Wesarg venit, vidit, dixit 15.06.2009 22:45:
Show 12 quoted lines
> 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.)

Show 7 quoted lines
>  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
Bert Wesarg· Jun 16, 2009, 07:56 UTC · re: Michael J Gruber · lore

Re: [PATCH] builtin-remote: (get_one_entry): use strbuf

On Tue, Jun 16, 2009 at 09:39, Michael J Gruber<git@drmicha.warpmail.net> wrote:
Show 16 quoted lines
> 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?"

Show 16 quoted lines
> 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
>
Michael J Gruber· Jun 16, 2009, 10:49 UTC · re: Bert Wesarg · lore

Re: [PATCH] builtin-remote: (get_one_entry): use strbuf

Bert Wesarg venit, vidit, dixit 16.06.2009 09:56:
Show 19 quoted lines
> 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.

Show 22 quoted lines
> 
>> 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
Bert Wesarg· Jun 22, 2009, 08:24 UTC · re: Michael J Gruber · lore

[PATCH] builtin-remote: (get_one_entry): use strbuf

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(-)
Show changes to builtin-remote.c +10 −11
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)
Jeff King· Jun 22, 2009, 21:32 UTC · re: Bert Wesarg · lore

Re: [PATCH] builtin-remote: (get_one_entry): use strbuf

On Mon, Jun 22, 2009 at 10:24:19AM +0200, Bert Wesarg wrote:
Show 24 quoted lines
> --- 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;
Show 12 quoted lines
> @@ -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
Bert Wesarg· Jun 22, 2009, 22:27 UTC · re: Jeff King · lore

[PATCH v2] builtin-remote: (get_one_entry): use strbuf

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(-)
Show changes to builtin-remote.c +7 −9
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)

← back to recent threads