threads / patch / 5418

patchparse_object: check if buffer is non-NULL before freeing it

Subject: [PATCH] parse_object: check if buffer is non-NULL before freeing it

## tl;dr

4 messages between Aug 28, 2006 and Aug 28, 2006. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Jonas Fonseca· Aug 28, 2006, 00:31 UTC · lore

Two code paths may cause this and other places in the source have the courtesy to do the check.

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 object.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to object.c +2 −1
diff --git a/object.c b/object.c
index 9281300..c08347f 100644
--- a/object.c
+++ b/object.c
@@ -173,7 +173,8 @@ struct object *parse_object(const unsign
 		} else {
 			obj = NULL;
 		}
-		free(buffer);
+		if (buffer)
+			free(buffer);
 		return obj;
 	}
 	return NULL;

-- 
1.4.2.g2f76-dirty

-- 
Jonas Fonseca
Junio C Hamano· Aug 28, 2006, 04:20 UTC · re: Jonas Fonseca · lore

Re: [PATCH] parse_object: check if buffer is non-NULL before freeing it

Jonas Fonseca <fonseca@diku.dk> writes:
> Two code paths may cause this and other places in the source have the
> courtesy to do the check.

Eh, free(NULL) should work just fine. It is "other places" that is misguided and needs to be fixed.

- >8 -
[PATCH] free(NULL) is perfectly valid.

Jonas noticed some places say "if (X) free(X)" which is totally unnecessary.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-apply.c         |    6 ++----
 builtin-fmt-merge-msg.c |    3 +--
 builtin-repo-config.c   |    6 ++----
 builtin-rev-list.c      |    6 ++----
 config.c                |    6 ++----
 fetch.c                 |    3 +--
 http-fetch.c            |   13 ++++---------
 http-push.c             |   13 ++++---------
 path-list.c             |    3 +--
 refs.c                  |    6 ++----
 10 files changed, 21 insertions(+), 44 deletions(-)
Show changes to 10 files +21 −44

builtin-apply.c, builtin-fmt-merge-msg.c, builtin-repo-config.c, builtin-rev-list.c, config.c, fetch.c, http-fetch.c, http-push.c, path-list.c, refs.c

diff --git a/builtin-apply.c b/builtin-apply.c
index b47ccac..1a1deaf 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1131,8 +1131,7 @@ static struct fragment *parse_binary_hun
 	return frag;
 
  corrupt:
-	if (data)
-		free(data);
+	free(data);
 	*status_p = -1;
 	error("corrupt binary patch at line %d: %.*s",
 	      linenr-1, llen-1, buffer);
@@ -1329,8 +1328,7 @@ static void show_stats(struct patch *pat
 		printf(" %s%-*s |%5d %.*s%.*s\n", prefix,
 		       len, name, patch->lines_added + patch->lines_deleted,
 		       add, pluses, del, minuses);
-	if (qname)
-		free(qname);
+	free(qname);
 }
 
 static int read_old_data(struct stat *st, const char *path, void *buf, unsigned long size)
diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index a5ed8db..76d22b4 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c
@@ -55,8 +55,7 @@ static void free_list(struct list *list)
 
 	for (i = 0; i < list->nr; i++) {
 		free(list->list[i]);
-		if (list->payload[i])
-			free(list->payload[i]);
+		free(list->payload[i]);
 	}
 	free(list->list);
 	free(list->payload);
diff --git a/builtin-repo-config.c b/builtin-repo-config.c
index c416480..6560cf1 100644
--- a/builtin-repo-config.c
+++ b/builtin-repo-config.c
@@ -122,10 +122,8 @@ static int get_value(const char* key_, c
 		ret =  (seen == 1) ? 0 : 1;
 
 free_strings:
-	if (repo_config)
-		free(repo_config);
-	if (global)
-		free(global);
+	free(repo_config);
+	free(global);
 	return ret;
 }
 
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index bc48a3e..7f3e1fc 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -93,10 +93,8 @@ static void show_commit(struct commit *c
 		free_commit_list(commit->parents);
 		commit->parents = NULL;
 	}
-	if (commit->buffer) {
-		free(commit->buffer);
-		commit->buffer = NULL;
-	}
+	free(commit->buffer);
+	commit->buffer = NULL;
 }
 
 static void process_blob(struct blob *blob,
diff --git a/config.c b/config.c
index 82b3562..d9f2b78 100644
--- a/config.c
+++ b/config.c
@@ -361,8 +361,7 @@ int git_config(config_fn_t fn)
 	}
 
 	ret += git_config_from_file(fn, filename);
-	if (repo_config)
-		free(repo_config);
+	free(repo_config);
 	return ret;
 }
 
@@ -734,8 +733,7 @@ int git_config_set_multivar(const char* 
 out_free:
 	if (0 <= fd)
 		close(fd);
-	if (config_filename)
-		free(config_filename);
+	free(config_filename);
 	if (lock_file) {
 		unlink(lock_file);
 		free(lock_file);
diff --git a/fetch.c b/fetch.c
index ef60b04..7d3812c 100644
--- a/fetch.c
+++ b/fetch.c
@@ -302,8 +302,7 @@ int pull(int targets, char **target, con
 		if (ret)
 			goto unlock_and_fail;
 	}
-	if (msg)
-		free(msg);
+	free(msg);
 
 	return 0;
 
diff --git a/http-fetch.c b/http-fetch.c
index 7619b33..6806f36 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -696,10 +696,8 @@ xml_start_tag(void *userData, const char
 	strcat(ctx->name, ".");
 	strcat(ctx->name, c);
 
-	if (ctx->cdata) {
-		free(ctx->cdata);
-		ctx->cdata = NULL;
-	}
+	free(ctx->cdata);
+	ctx->cdata = NULL;
 
 	ctx->userFunc(ctx, 0);
 }
@@ -726,8 +724,7 @@ static void
 xml_cdata(void *userData, const XML_Char *s, int len)
 {
 	struct xml_ctx *ctx = (struct xml_ctx *)userData;
-	if (ctx->cdata)
-		free(ctx->cdata);
+	free(ctx->cdata);
 	ctx->cdata = xmalloc(len + 1);
 	strlcpy(ctx->cdata, s, len + 1);
 }
@@ -765,9 +762,7 @@ static void handle_remote_ls_ctx(struct 
 			ls->dentry_flags |= IS_DIR;
 		}
 	} else if (!strcmp(ctx->name, DAV_PROPFIND_RESP)) {
-		if (ls->dentry_name) {
-			free(ls->dentry_name);
-		}
+		free(ls->dentry_name);
 		ls->dentry_name = NULL;
 		ls->dentry_flags = 0;
 	}
diff --git a/http-push.c b/http-push.c
index 04cb238..7814666 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1238,10 +1238,8 @@ xml_start_tag(void *userData, const char
 	strcat(ctx->name, ".");
 	strcat(ctx->name, c);
 
-	if (ctx->cdata) {
-		free(ctx->cdata);
-		ctx->cdata = NULL;
-	}
+	free(ctx->cdata);
+	ctx->cdata = NULL;
 
 	ctx->userFunc(ctx, 0);
 }
@@ -1268,8 +1266,7 @@ static void
 xml_cdata(void *userData, const XML_Char *s, int len)
 {
 	struct xml_ctx *ctx = (struct xml_ctx *)userData;
-	if (ctx->cdata)
-		free(ctx->cdata);
+	free(ctx->cdata);
 	ctx->cdata = xmalloc(len + 1);
 	strlcpy(ctx->cdata, s, len + 1);
 }
@@ -1518,9 +1515,7 @@ static void handle_remote_ls_ctx(struct 
 			ls->dentry_flags |= IS_DIR;
 		}
 	} else if (!strcmp(ctx->name, DAV_PROPFIND_RESP)) {
-		if (ls->dentry_name) {
-			free(ls->dentry_name);
-		}
+		free(ls->dentry_name);
 		ls->dentry_name = NULL;
 		ls->dentry_flags = 0;
 	}
diff --git a/path-list.c b/path-list.c
index f15a10d..b1ee72d 100644
--- a/path-list.c
+++ b/path-list.c
@@ -85,8 +85,7 @@ void path_list_clear(struct path_list *l
 			for (i = 0; i < list->nr; i++) {
 				if (list->strdup_paths)
 					free(list->items[i].path);
-				if (list->items[i].util)
-					free(list->items[i].util);
+				free(list->items[i].util);
 			}
 		free(list->items);
 	}
diff --git a/refs.c b/refs.c
index e70ef0a..aab14fc 100644
--- a/refs.c
+++ b/refs.c
@@ -348,10 +348,8 @@ void unlock_ref(struct ref_lock *lock)
 		if (lock->lk)
 			rollback_lock_file(lock->lk);
 	}
-	if (lock->ref_file)
-		free(lock->ref_file);
-	if (lock->log_file)
-		free(lock->log_file);
+	free(lock->ref_file);
+	free(lock->log_file);
 	free(lock);
 }
 

-- 
1.4.2.g2f76
Linus Torvalds· Aug 28, 2006, 04:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] parse_object: check if buffer is non-NULL before freeing it

On Sun, 27 Aug 2006, Junio C Hamano wrote:
> 
> Eh, free(NULL) should work just fine.  It is "other places" that
> is misguided and needs to be fixed.
Well, some very old libraries will SIGSEGV on free(NULL).

Admittedly those libraries are either very old or _very_ broken, but if you want to be strictly portable, you should not ever pass NULL to free(), unless you actually got it from a malloc(0) (and even then, it might be a really broken libc that just ran out of memory).

I actually suspect we should wrap all free() calls as "xfree()", which may also help us some day if we want to do any memory usage statistics.

		Linus
Junio C Hamano· Aug 28, 2006, 05:46 UTC · re: Linus Torvalds · lore

Re: [PATCH] parse_object: check if buffer is non-NULL before freeing it

Linus Torvalds <torvalds@osdl.org> writes:
Show 11 quoted lines
> On Sun, 27 Aug 2006, Junio C Hamano wrote:
>> 
>> Eh, free(NULL) should work just fine.  It is "other places" that
>> is misguided and needs to be fixed.
>
> Well, some very old libraries will SIGSEGV on free(NULL). 
>
> Admittedly those libraries are either very old or _very_ broken, but if 
> you want to be strictly portable, you should not ever pass NULL to free(), 
> unless you actually got it from a malloc(0) (and even then, it might be a 
> really broken libc that just ran out of memory).

Fair enough, but I think there are many places we already assume the library handles free(NULL) sensibly.

> I actually suspect we should wrap all free() calls as "xfree()", which may 
> also help us some day if we want to do any memory usage statistics.
That sounds sensible.

Another thing I was thinking about was to extend the existing XMALLOC_POISON debugging to allow also xrealloc()'ed area. That would unfortunately involve wrapping strdup() and x*alloc() to make sure all allocations we do go through xmalloc() and then store the current allocation size somewhere hidden (immediately before, perhaps) in the area xmalloc() returns, but at that point running git under valgrind would probably be easier.

← back to recent threads