[PATCH] Check return value of ftruncate call in http.c. Remove possible mem leak

Subsystems: the rest

STALE3738d

6 messages, 3 authors, 2016-06-15 · open the first message on its own page

[PATCH] Check return value of ftruncate call in http.c. Remove possible mem leak

From: Jeff Lasslett <hidden>
Date: 2016-06-15 22:47:12

In new_http_object_request(), check ftruncate() call return value and
handle possible errors.  Remove possible leak of mem pointed to by url.

Signed-off-by: Jeff Lasslett <redacted>
---
 http.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index a2720d5..e8317e1 100644
--- a/http.c
+++ b/http.c

@@ -1098,7 +1098,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	char *hex = sha1_to_hex(sha1);
 	char *filename;
 	char prevfile[PATH_MAX];
-	char *url;
+	char *url = NULL;
 	int prevlocal;
 	unsigned char prev_buf[PREV_BUF_SIZE];
 	ssize_t prev_read = 0;

@@ -1189,7 +1189,11 @@ struct http_object_request *new_http_object_request(const char *base_url,
 		if (prev_posn>0) {
 			prev_posn = 0;
 			lseek(freq->localfile, 0, SEEK_SET);
-			ftruncate(freq->localfile, 0);
+			if (ftruncate(freq->localfile, 0) < 0) {
+				error("Couldn't truncate temporary file %s for %s: %s",
+					  freq->tmpfile, freq->filename, strerror(errno));
+				goto abort;
+			}
 		}
 	}

@@ -1198,7 +1202,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);
-	curl_easy_setopt(freq->slot->curl, CURLOPT_URL, url);
+	curl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);
 
 	/*

@@ -1216,10 +1220,12 @@ struct http_object_request *new_http_object_request(const char *base_url,
 				 CURLOPT_HTTPHEADER, range_header);
 	}
 
+	free(url);
 	return freq;
 
-	free(url);
 abort:
+	free(url);
+	free(freq->url);
 	free(filename);
 	free(freq);
 	return NULL;

-- 
1.6.4.59.g4d590.dirty

Re: [PATCH] Check return value of ftruncate call in http.c. Remove possible mem leak

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:47:13

Hi,

On Mon, Aug 10, 2009 at 9:42 PM, Jeff Lasslett[off-list ref] wrote:

quoted hunk

@@ -1216,10 +1220,12 @@ struct http_object_request *new_http_object_request(const char *base_url,

                                CURLOPT_HTTPHEADER, range_header);
       }

+       free(url);
       return freq;

-       free(url);

Freeing after the return didn't make sense. Thanks for spotting this.

On a side note, you could simply do away with the 'url' variable,
since you're going to pass freq->url to curl_setopt.

-- 
Cheers,
Ray Chuan

[PATCH (resend) 1/3] http.c: free preq when aborting

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:47:13

Free preq in new_http_pack_request when aborting. preq was allocated
before jumping to the 'abort' label so this is safe.

Signed-off-by: Tay Ray Chuan <redacted>
---
 http.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/http.c b/http.c
index a2720d5..cfe32f5 100644
--- a/http.c
+++ b/http.c

@@ -1059,6 +1059,7 @@ struct http_pack_request *new_http_pack_request(

 abort:
 	free(filename);
+	free(preq);
 	return NULL;
 }

--

1.6.3.1

[PATCH (resend) 2/3] http.c: replace usage of temporary variable for urls

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:47:13

Use preq->url in new_http_pack_request and freq->url in
new_http_object_request when calling curl_setopt(CURLOPT_URL), instead
of using an intermediate variable, 'url'.

Signed-off-by: Tay Ray Chuan <redacted>
---
 http.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/http.c b/http.c
index cfe32f5..98f9707 100644
--- a/http.c
+++ b/http.c

@@ -1004,7 +1004,6 @@ int finish_http_pack_request(struct http_pack_request *preq)
 struct http_pack_request *new_http_pack_request(
 	struct packed_git *target, const char *base_url)
 {
-	char *url;
 	char *filename;
 	long prev_posn = 0;
 	char range[RANGE_HEADER_SIZE];

@@ -1018,8 +1017,7 @@ struct http_pack_request *new_http_pack_request(
 	end_url_with_slash(&buf, base_url);
 	strbuf_addf(&buf, "objects/pack/pack-%s.pack",
 		sha1_to_hex(target->sha1));
-	url = strbuf_detach(&buf, NULL);
-	preq->url = xstrdup(url);
+	preq->url = strbuf_detach(&buf, NULL);

 	filename = sha1_pack_name(target->sha1);
 	snprintf(preq->filename, sizeof(preq->filename), "%s", filename);

@@ -1035,7 +1033,7 @@ struct http_pack_request *new_http_pack_request(
 	preq->slot->local = preq->packfile;
 	curl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
-	curl_easy_setopt(preq->slot->curl, CURLOPT_URL, url);
+	curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,
 		no_pragma_header);

@@ -1059,6 +1057,7 @@ struct http_pack_request *new_http_pack_request(

 abort:
 	free(filename);
+	free(preq->url);
 	free(preq);
 	return NULL;
 }

@@ -1099,7 +1098,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	char *hex = sha1_to_hex(sha1);
 	char *filename;
 	char prevfile[PATH_MAX];
-	char *url;
 	int prevlocal;
 	unsigned char prev_buf[PREV_BUF_SIZE];
 	ssize_t prev_read = 0;

@@ -1153,8 +1151,7 @@ struct http_object_request *new_http_object_request(const char *base_url,

 	git_SHA1_Init(&freq->c);

-	url = get_remote_object_url(base_url, hex, 0);
-	freq->url = xstrdup(url);
+	freq->url = get_remote_object_url(base_url, hex, 0);

 	/*
 	 * If a previous temp file is present, process what was already

@@ -1199,7 +1196,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);
-	curl_easy_setopt(freq->slot->curl, CURLOPT_URL, url);
+	curl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);

 	/*

@@ -1219,9 +1216,9 @@ struct http_object_request *new_http_object_request(const char *base_url,

 	return freq;

-	free(url);
 abort:
 	free(filename);
+	free(freq->url);
 	free(freq);
 	return NULL;
 }
--

1.6.3.1

[PATCH (resend) 3/3] Check return value of ftruncate call in http.c

From: Tay Ray Chuan <hidden>
Date: 2016-06-15 22:47:13

From: Jeff Lasslett <redacted>

In new_http_object_request(), check ftruncate() call return value and
handle possible errors.

Signed-off-by: Jeff Lasslett <redacted>
Signed-off-by: Tay Ray Chuan <redacted>
---

 Jeff, I took out all the url-specific cleanups and expanded their
 scope (see earlier patches). You can safely focus on ftruncate() and
 do an abort there. :)

 http.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 98f9707..14d5357 100644
--- a/http.c
+++ b/http.c

@@ -1187,7 +1187,11 @@ struct http_object_request *new_http_object_request(const char *base_url,
 		if (prev_posn>0) {
 			prev_posn = 0;
 			lseek(freq->localfile, 0, SEEK_SET);
-			ftruncate(freq->localfile, 0);
+			if (ftruncate(freq->localfile, 0) < 0) {
+				error("Couldn't truncate temporary file %s for %s: %s",
+					  freq->tmpfile, freq->filename, strerror(errno));
+				goto abort;
+			}
 		}
 	}

--

1.6.3.1

Re: [PATCH (resend) 3/3] Check return value of ftruncate call in http.c

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:13

Thanks; all three patches queued.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help