{"thread":{"id":"5418","subject":"[PATCH] parse_object: check if buffer is non-NULL before freeing it","startedAt":"2006-08-28T00:31:29Z","lastAt":"2006-08-28T05:46:09Z","messageCount":4,"participants":["Jonas Fonseca","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"26020","messageId":"20060828003129.GE20904@diku.dk","threadId":"5418","inReplyTo":null,"subject":"[PATCH] parse_object: check if buffer is non-NULL before freeing it","fromName":"Jonas Fonseca","fromEmail":"fonseca@diku.dk","sentAt":"2006-08-28T00:31:29Z","receivedAt":"2006-08-28T00:31:29Z","isPatch":true,"sender":{"key":"fonseca@diku.dk","avatar":"https://gravatar.com/avatar/f82f3ad698717c51873b020c750a92438c820a24056dc39fe4d07baa10a92264?d=mp&s=160"},"body":"Two code paths may cause this and other places in the source have the\ncourtesy to do the check.\n\nSigned-off-by: Jonas Fonseca <fonseca@diku.dk>\n---\n object.c |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 9281300..c08347f 100644\n--- a/object.c\n+++ b/object.c\n@@ -173,7 +173,8 @@ struct object *parse_object(const unsign\n \t\t} else {\n \t\t\tobj = NULL;\n \t\t}\n-\t\tfree(buffer);\n+\t\tif (buffer)\n+\t\t\tfree(buffer);\n \t\treturn obj;\n \t}\n \treturn NULL;\n-- \n1.4.2.g2f76-dirty\n\n-- \nJonas Fonseca\n"},{"id":"26024","messageId":"7vsljhtrsv.fsf@assigned-by-dhcp.cox.net","threadId":"5418","inReplyTo":"20060828003129.GE20904@diku.dk","subject":"Re: [PATCH] parse_object: check if buffer is non-NULL before freeing it","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-28T04:20:16Z","receivedAt":"2006-08-28T04:20:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonas Fonseca <fonseca@diku.dk> writes:\n\n> Two code paths may cause this and other places in the source have the\n> courtesy to do the check.\n\nEh, free(NULL) should work just fine.  It is \"other places\" that\nis misguided and needs to be fixed.\n\n- >8 -\n[PATCH] free(NULL) is perfectly valid.\n\nJonas noticed some places say \"if (X) free(X)\" which is totally\nunnecessary.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n builtin-apply.c         |    6 ++----\n builtin-fmt-merge-msg.c |    3 +--\n builtin-repo-config.c   |    6 ++----\n builtin-rev-list.c      |    6 ++----\n config.c                |    6 ++----\n fetch.c                 |    3 +--\n http-fetch.c            |   13 ++++---------\n http-push.c             |   13 ++++---------\n path-list.c             |    3 +--\n refs.c                  |    6 ++----\n 10 files changed, 21 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex b47ccac..1a1deaf 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1131,8 +1131,7 @@ static struct fragment *parse_binary_hun\n \treturn frag;\n \n  corrupt:\n-\tif (data)\n-\t\tfree(data);\n+\tfree(data);\n \t*status_p = -1;\n \terror(\"corrupt binary patch at line %d: %.*s\",\n \t      linenr-1, llen-1, buffer);\n@@ -1329,8 +1328,7 @@ static void show_stats(struct patch *pat\n \t\tprintf(\" %s%-*s |%5d %.*s%.*s\\n\", prefix,\n \t\t       len, name, patch->lines_added + patch->lines_deleted,\n \t\t       add, pluses, del, minuses);\n-\tif (qname)\n-\t\tfree(qname);\n+\tfree(qname);\n }\n \n static int read_old_data(struct stat *st, const char *path, void *buf, unsigned long size)\ndiff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c\nindex a5ed8db..76d22b4 100644\n--- a/builtin-fmt-merge-msg.c\n+++ b/builtin-fmt-merge-msg.c\n@@ -55,8 +55,7 @@ static void free_list(struct list *list)\n \n \tfor (i = 0; i < list->nr; i++) {\n \t\tfree(list->list[i]);\n-\t\tif (list->payload[i])\n-\t\t\tfree(list->payload[i]);\n+\t\tfree(list->payload[i]);\n \t}\n \tfree(list->list);\n \tfree(list->payload);\ndiff --git a/builtin-repo-config.c b/builtin-repo-config.c\nindex c416480..6560cf1 100644\n--- a/builtin-repo-config.c\n+++ b/builtin-repo-config.c\n@@ -122,10 +122,8 @@ static int get_value(const char* key_, c\n \t\tret =  (seen == 1) ? 0 : 1;\n \n free_strings:\n-\tif (repo_config)\n-\t\tfree(repo_config);\n-\tif (global)\n-\t\tfree(global);\n+\tfree(repo_config);\n+\tfree(global);\n \treturn ret;\n }\n \ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex bc48a3e..7f3e1fc 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -93,10 +93,8 @@ static void show_commit(struct commit *c\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n \t}\n-\tif (commit->buffer) {\n-\t\tfree(commit->buffer);\n-\t\tcommit->buffer = NULL;\n-\t}\n+\tfree(commit->buffer);\n+\tcommit->buffer = NULL;\n }\n \n static void process_blob(struct blob *blob,\ndiff --git a/config.c b/config.c\nindex 82b3562..d9f2b78 100644\n--- a/config.c\n+++ b/config.c\n@@ -361,8 +361,7 @@ int git_config(config_fn_t fn)\n \t}\n \n \tret += git_config_from_file(fn, filename);\n-\tif (repo_config)\n-\t\tfree(repo_config);\n+\tfree(repo_config);\n \treturn ret;\n }\n \n@@ -734,8 +733,7 @@ int git_config_set_multivar(const char* \n out_free:\n \tif (0 <= fd)\n \t\tclose(fd);\n-\tif (config_filename)\n-\t\tfree(config_filename);\n+\tfree(config_filename);\n \tif (lock_file) {\n \t\tunlink(lock_file);\n \t\tfree(lock_file);\ndiff --git a/fetch.c b/fetch.c\nindex ef60b04..7d3812c 100644\n--- a/fetch.c\n+++ b/fetch.c\n@@ -302,8 +302,7 @@ int pull(int targets, char **target, con\n \t\tif (ret)\n \t\t\tgoto unlock_and_fail;\n \t}\n-\tif (msg)\n-\t\tfree(msg);\n+\tfree(msg);\n \n \treturn 0;\n \ndiff --git a/http-fetch.c b/http-fetch.c\nindex 7619b33..6806f36 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -696,10 +696,8 @@ xml_start_tag(void *userData, const char\n \tstrcat(ctx->name, \".\");\n \tstrcat(ctx->name, c);\n \n-\tif (ctx->cdata) {\n-\t\tfree(ctx->cdata);\n-\t\tctx->cdata = NULL;\n-\t}\n+\tfree(ctx->cdata);\n+\tctx->cdata = NULL;\n \n \tctx->userFunc(ctx, 0);\n }\n@@ -726,8 +724,7 @@ static void\n xml_cdata(void *userData, const XML_Char *s, int len)\n {\n \tstruct xml_ctx *ctx = (struct xml_ctx *)userData;\n-\tif (ctx->cdata)\n-\t\tfree(ctx->cdata);\n+\tfree(ctx->cdata);\n \tctx->cdata = xmalloc(len + 1);\n \tstrlcpy(ctx->cdata, s, len + 1);\n }\n@@ -765,9 +762,7 @@ static void handle_remote_ls_ctx(struct \n \t\t\tls->dentry_flags |= IS_DIR;\n \t\t}\n \t} else if (!strcmp(ctx->name, DAV_PROPFIND_RESP)) {\n-\t\tif (ls->dentry_name) {\n-\t\t\tfree(ls->dentry_name);\n-\t\t}\n+\t\tfree(ls->dentry_name);\n \t\tls->dentry_name = NULL;\n \t\tls->dentry_flags = 0;\n \t}\ndiff --git a/http-push.c b/http-push.c\nindex 04cb238..7814666 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1238,10 +1238,8 @@ xml_start_tag(void *userData, const char\n \tstrcat(ctx->name, \".\");\n \tstrcat(ctx->name, c);\n \n-\tif (ctx->cdata) {\n-\t\tfree(ctx->cdata);\n-\t\tctx->cdata = NULL;\n-\t}\n+\tfree(ctx->cdata);\n+\tctx->cdata = NULL;\n \n \tctx->userFunc(ctx, 0);\n }\n@@ -1268,8 +1266,7 @@ static void\n xml_cdata(void *userData, const XML_Char *s, int len)\n {\n \tstruct xml_ctx *ctx = (struct xml_ctx *)userData;\n-\tif (ctx->cdata)\n-\t\tfree(ctx->cdata);\n+\tfree(ctx->cdata);\n \tctx->cdata = xmalloc(len + 1);\n \tstrlcpy(ctx->cdata, s, len + 1);\n }\n@@ -1518,9 +1515,7 @@ static void handle_remote_ls_ctx(struct \n \t\t\tls->dentry_flags |= IS_DIR;\n \t\t}\n \t} else if (!strcmp(ctx->name, DAV_PROPFIND_RESP)) {\n-\t\tif (ls->dentry_name) {\n-\t\t\tfree(ls->dentry_name);\n-\t\t}\n+\t\tfree(ls->dentry_name);\n \t\tls->dentry_name = NULL;\n \t\tls->dentry_flags = 0;\n \t}\ndiff --git a/path-list.c b/path-list.c\nindex f15a10d..b1ee72d 100644\n--- a/path-list.c\n+++ b/path-list.c\n@@ -85,8 +85,7 @@ void path_list_clear(struct path_list *l\n \t\t\tfor (i = 0; i < list->nr; i++) {\n \t\t\t\tif (list->strdup_paths)\n \t\t\t\t\tfree(list->items[i].path);\n-\t\t\t\tif (list->items[i].util)\n-\t\t\t\t\tfree(list->items[i].util);\n+\t\t\t\tfree(list->items[i].util);\n \t\t\t}\n \t\tfree(list->items);\n \t}\ndiff --git a/refs.c b/refs.c\nindex e70ef0a..aab14fc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -348,10 +348,8 @@ void unlock_ref(struct ref_lock *lock)\n \t\tif (lock->lk)\n \t\t\trollback_lock_file(lock->lk);\n \t}\n-\tif (lock->ref_file)\n-\t\tfree(lock->ref_file);\n-\tif (lock->log_file)\n-\t\tfree(lock->log_file);\n+\tfree(lock->ref_file);\n+\tfree(lock->log_file);\n \tfree(lock);\n }\n \n-- \n1.4.2.g2f76\n"},{"id":"26026","messageId":"Pine.LNX.4.64.0608272131020.27779@g5.osdl.org","threadId":"5418","inReplyTo":"7vsljhtrsv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] parse_object: check if buffer is non-NULL before freeing it","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-08-28T04:33:16Z","receivedAt":"2006-08-28T04:33:16Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 27 Aug 2006, Junio C Hamano wrote:\n> \n> Eh, free(NULL) should work just fine.  It is \"other places\" that\n> is misguided and needs to be fixed.\n\nWell, some very old libraries will SIGSEGV on free(NULL). \n\nAdmittedly those libraries are either very old or _very_ broken, but if \nyou want to be strictly portable, you should not ever pass NULL to free(), \nunless you actually got it from a malloc(0) (and even then, it might be a \nreally broken libc that just ran out of memory).\n\nI actually suspect we should wrap all free() calls as \"xfree()\", which may \nalso help us some day if we want to do any memory usage statistics.\n\n\t\tLinus\n"},{"id":"26031","messageId":"7v3bbhtntq.fsf@assigned-by-dhcp.cox.net","threadId":"5418","inReplyTo":"Pine.LNX.4.64.0608272131020.27779@g5.osdl.org","subject":"Re: [PATCH] parse_object: check if buffer is non-NULL before freeing it","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-28T05:46:09Z","receivedAt":"2006-08-28T05:46:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> On Sun, 27 Aug 2006, Junio C Hamano wrote:\n>> \n>> Eh, free(NULL) should work just fine.  It is \"other places\" that\n>> is misguided and needs to be fixed.\n>\n> Well, some very old libraries will SIGSEGV on free(NULL). \n>\n> Admittedly those libraries are either very old or _very_ broken, but if \n> you want to be strictly portable, you should not ever pass NULL to free(), \n> unless you actually got it from a malloc(0) (and even then, it might be a \n> really broken libc that just ran out of memory).\n\nFair enough, but I think there are many places we already assume\nthe library handles free(NULL) sensibly.\n\n> I actually suspect we should wrap all free() calls as \"xfree()\", which may \n> also help us some day if we want to do any memory usage statistics.\n\nThat sounds sensible.\n\nAnother thing I was thinking about was to extend the existing\nXMALLOC_POISON debugging to allow also xrealloc()'ed area.  That\nwould unfortunately involve wrapping strdup() and x*alloc() to\nmake sure all allocations we do go through xmalloc() and then\nstore the current allocation size somewhere hidden (immediately\nbefore, perhaps) in the area xmalloc() returns, but at that\npoint running git under valgrind would probably be easier.\n"}]}