{"thread":{"id":"20518","subject":"[PATCH] Check return value of ftruncate call in http.c. Remove possible mem leak","startedAt":"2009-08-10T13:42:27Z","lastAt":"2009-08-11T05:13:14Z","messageCount":6,"participants":["Jeff Lasslett","Tay Ray Chuan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"120149","messageId":"1249911747-28490-1-git-send-email-jeff.lasslett@gmail.com","threadId":"20518","inReplyTo":null,"subject":"[PATCH] Check return value of ftruncate call in http.c. Remove possible mem leak","fromName":"Jeff Lasslett","fromEmail":"jeff.lasslett@gmail.com","sentAt":"2009-08-10T13:42:27Z","receivedAt":"2009-08-10T13:42:27Z","isPatch":true,"sender":{"key":"jeff.lasslett@gmail.com","avatar":null},"body":"In new_http_object_request(), check ftruncate() call return value and\nhandle possible errors. Remove possible leak of mem pointed to by url.\n\nSigned-off-by: Jeff Lasslett \n---\n http.c | 14 ++++++++++----\n 1 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a2720d5..e8317e1 100644\n--- a/http.c\n+++ b/http.c\n@@ -1098,7 +1098,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tchar *hex = sha1_to_hex(sha1);\n \tchar *filename;\n \tchar prevfile[PATH_MAX];\n-\tchar *url;\n+\tchar *url = NULL;\n \tint prevlocal;\n \tunsigned char prev_buf[PREV_BUF_SIZE];\n \tssize_t prev_read = 0;\n@@ -1189,7 +1189,11 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t\tif (prev_posn>0) {\n \t\t\tprev_posn = 0;\n \t\t\tlseek(freq->localfile, 0, SEEK_SET);\n-\t\t\tftruncate(freq->localfile, 0);\n+\t\t\tif (ftruncate(freq->localfile, 0) < 0) {\n+\t\t\t\terror(\"Couldn't truncate temporary file %s for %s: %s\",\n+\t\t\t\t\t freq->tmpfile, freq->filename, strerror(errno));\n+\t\t\t\tgoto abort;\n+\t\t\t}\n \t\t}\n \t}\n \n@@ -1198,7 +1202,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);\n-\tcurl_easy_setopt(freq->slot->curl, CURLOPT_URL, url);\n+\tcurl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);\n \n \t/*\n@@ -1216,10 +1220,12 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t\t\t\t CURLOPT_HTTPHEADER, range_header);\n \t}\n \n+\tfree(url);\n \treturn freq;\n \n-\tfree(url);\n abort:\n+\tfree(url);\n+\tfree(freq->url);\n \tfree(filename);\n \tfree(freq);\n \treturn NULL;\n-- \n1.6.4.59.g4d590.dirty\n"},{"id":"120174","messageId":"be6fef0d0908100847h7e9b53a2ofdf318e9958be3b2@mail.gmail.com","threadId":"20518","inReplyTo":"1249911747-28490-1-git-send-email-jeff.lasslett@gmail.com","subject":"Re: [PATCH] Check return value of ftruncate call in http.c. Remove possible mem leak","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-08-10T15:47:20Z","receivedAt":"2009-08-10T15:47:20Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Mon, Aug 10, 2009 at 9:42 PM, Jeff Lasslett wrote:\n> @@ -1216,10 +1220,12 @@ struct http_object_request *new_http_object_request(const char *base_url,\n>                                 CURLOPT_HTTPHEADER, range_header);\n>        }\n>\n> +       free(url);\n>        return freq;\n>\n> -       free(url);\n\nFreeing after the return didn't make sense. Thanks for spotting this.\n\nOn a side note, you could simply do away with the 'url' variable,\nsince you're going to pass freq->url to curl_setopt.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"120178","messageId":"20090810235548.110c43a6.rctay89@gmail.com","threadId":"20518","inReplyTo":"be6fef0d0908100847h7e9b53a2ofdf318e9958be3b2@mail.gmail.com","subject":"[PATCH (resend) 1/3] http.c: free preq when aborting","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-08-10T15:55:48Z","receivedAt":"2009-08-10T15:55:48Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Free preq in new_http_pack_request when aborting. preq was allocated\nbefore jumping to the 'abort' label so this is safe.\n\nSigned-off-by: Tay Ray Chuan \n---\n http.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a2720d5..cfe32f5 100644\n--- a/http.c\n+++ b/http.c\n@@ -1059,6 +1059,7 @@ struct http_pack_request *new_http_pack_request(\n\n abort:\n \tfree(filename);\n+\tfree(preq);\n \treturn NULL;\n }\n\n--\n1.6.3.1\n"},{"id":"120180","messageId":"20090810235955.1a659e6c.rctay89@gmail.com","threadId":"20518","inReplyTo":"20090810235548.110c43a6.rctay89@gmail.com","subject":"[PATCH (resend) 2/3] http.c: replace usage of temporary variable for urls","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-08-10T15:59:55Z","receivedAt":"2009-08-10T15:59:55Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Use preq->url in new_http_pack_request and freq->url in\nnew_http_object_request when calling curl_setopt(CURLOPT_URL), instead\nof using an intermediate variable, 'url'.\n\nSigned-off-by: Tay Ray Chuan \n---\n http.c | 15 ++++++---------\n 1 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex cfe32f5..98f9707 100644\n--- a/http.c\n+++ b/http.c\n@@ -1004,7 +1004,6 @@ int finish_http_pack_request(struct http_pack_request *preq)\n struct http_pack_request *new_http_pack_request(\n \tstruct packed_git *target, const char *base_url)\n {\n-\tchar *url;\n \tchar *filename;\n \tlong prev_posn = 0;\n \tchar range[RANGE_HEADER_SIZE];\n@@ -1018,8 +1017,7 @@ struct http_pack_request *new_http_pack_request(\n \tend_url_with_slash(&buf, base_url);\n \tstrbuf_addf(&buf, \"objects/pack/pack-%s.pack\",\n \t\tsha1_to_hex(target->sha1));\n-\turl = strbuf_detach(&buf, NULL);\n-\tpreq->url = xstrdup(url);\n+\tpreq->url = strbuf_detach(&buf, NULL);\n\n \tfilename = sha1_pack_name(target->sha1);\n \tsnprintf(preq->filename, sizeof(preq->filename), \"%s\", filename);\n@@ -1035,7 +1033,7 @@ struct http_pack_request *new_http_pack_request(\n \tpreq->slot->local = preq->packfile;\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);\n-\tcurl_easy_setopt(preq->slot->curl, CURLOPT_URL, url);\n+\tcurl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,\n \t\tno_pragma_header);\n\n@@ -1059,6 +1057,7 @@ struct http_pack_request *new_http_pack_request(\n\n abort:\n \tfree(filename);\n+\tfree(preq->url);\n \tfree(preq);\n \treturn NULL;\n }\n@@ -1099,7 +1098,6 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tchar *hex = sha1_to_hex(sha1);\n \tchar *filename;\n \tchar prevfile[PATH_MAX];\n-\tchar *url;\n \tint prevlocal;\n \tunsigned char prev_buf[PREV_BUF_SIZE];\n \tssize_t prev_read = 0;\n@@ -1153,8 +1151,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n\n \tgit_SHA1_Init(&freq->c);\n\n-\turl = get_remote_object_url(base_url, hex, 0);\n-\tfreq->url = xstrdup(url);\n+\tfreq->url = get_remote_object_url(base_url, hex, 0);\n\n \t/*\n \t * If a previous temp file is present, process what was already\n@@ -1199,7 +1196,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);\n-\tcurl_easy_setopt(freq->slot->curl, CURLOPT_URL, url);\n+\tcurl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);\n\n \t/*\n@@ -1219,9 +1216,9 @@ struct http_object_request *new_http_object_request(const char *base_url,\n\n \treturn freq;\n\n-\tfree(url);\n abort:\n \tfree(filename);\n+\tfree(freq->url);\n \tfree(freq);\n \treturn NULL;\n }\n--\n1.6.3.1\n"},{"id":"120181","messageId":"20090811000506.c63eb8f1.rctay89@gmail.com","threadId":"20518","inReplyTo":"20090810235955.1a659e6c.rctay89@gmail.com","subject":"[PATCH (resend) 3/3] Check return value of ftruncate call in http.c","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2009-08-10T16:05:06Z","receivedAt":"2009-08-10T16:05:06Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"From: Jeff Lasslett \n\nIn new_http_object_request(), check ftruncate() call return value and\nhandle possible errors.\n\nSigned-off-by: Jeff Lasslett \nSigned-off-by: Tay Ray Chuan \n---\n\n Jeff, I took out all the url-specific cleanups and expanded their\n scope (see earlier patches). You can safely focus on ftruncate() and\n do an abort there. :)\n\n http.c | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 98f9707..14d5357 100644\n--- a/http.c\n+++ b/http.c\n@@ -1187,7 +1187,11 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t\tif (prev_posn>0) {\n \t\t\tprev_posn = 0;\n \t\t\tlseek(freq->localfile, 0, SEEK_SET);\n-\t\t\tftruncate(freq->localfile, 0);\n+\t\t\tif (ftruncate(freq->localfile, 0) < 0) {\n+\t\t\t\terror(\"Couldn't truncate temporary file %s for %s: %s\",\n+\t\t\t\t\t freq->tmpfile, freq->filename, strerror(errno));\n+\t\t\t\tgoto abort;\n+\t\t\t}\n \t\t}\n \t}\n\n--\n1.6.3.1\n"},{"id":"120231","messageId":"7v63cvorv9.fsf@alter.siamese.dyndns.org","threadId":"20518","inReplyTo":"20090811000506.c63eb8f1.rctay89@gmail.com","subject":"Re: [PATCH (resend) 3/3] Check return value of ftruncate call in http.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-11T05:13:14Z","receivedAt":"2009-08-11T05:13:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; all three patches queued.\n"}]}