threads / patch / 11594

patchFix decompress_next_from() wrong argument value

Subject: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value

## tl;dr

6 messages between Jan 11, 2008 and Jan 12, 2008. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Marco Costalba· Jan 11, 2008, 20:47 UTC · lore

Function decompress_next_from() needs a pointer to a buffer and the buffer size as arguments.

Interesting enough the function fill() that returns the buffer pointer happens to modify also the buffer size, stored in a variable at file scope.

So we need to guarantee fill() is called before to use buffer size as argument in decompress_next_from()

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
Patch to be applied above decompress helper series.

Not to be pedantic, but have a function that gives two really coupled values, as a buffer pointer and the size, the first as return value and the second through a variable at file scope is not something you are going to see advertised in the programming books!

Sorry for this little rant but this bug really made me crazy.
With this patch 'make test' runs with success!
 builtin-unpack-objects.c |    3 ++-
 index-pack.c             |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)
Show changes to 2 files +4 −2

builtin-unpack-objects.c, index-pack.c

diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index f1a4883..72293ec 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -68,7 +68,8 @@ static void *get_data(unsigned long size)
 	decompress_into(&stream, buf, size);

 	for (;;) {
-		int ret = decompress_next_from(&stream, fill(1), len, Z_NO_FLUSH);
+		void* tmp = fill(1); // fill() modifies len, so be sure is evaluated as first
+		int ret = decompress_next_from(&stream, tmp, len, Z_NO_FLUSH);
 		use(len - stream.avail_in);
 		if (stream.total_out == size && ret == Z_STREAM_END)
 			break;
diff --git a/index-pack.c b/index-pack.c
index 30d7837..13b308d 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -173,7 +173,8 @@ static void *unpack_entry_data(unsigned long
offset, unsigned long size)
 	decompress_into(&stream, buf, size);

 	for (;;) {
-		int ret = decompress_next_from(&stream, fill(1), input_len, Z_NO_FLUSH);
+		void* tmp = fill(1); // fill() modifies input_len, so be sure is
evaluated as first
+		int ret = decompress_next_from(&stream, tmp, input_len, Z_NO_FLUSH);
 		use(input_len - stream.avail_in);
 		if (stream.total_out == size && ret == Z_STREAM_END)
 			break;
-- 
1.5.4.rc2.95.g0eaa-dirty
Junio C Hamano· Jan 12, 2008, 00:16 UTC · re: Marco Costalba · lore

Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value

"Marco Costalba" <mcostalba@gmail.com> writes:
> Patch to be applied above decompress helper series.

No way. That will mean that the resulting series will start with a known bug.

Show 6 quoted lines
> Not to be pedantic, but have a function that gives two really
> coupled values, as a buffer pointer and the size, the first as return
> value and the second through a variable at file scope is not something
> you are going to see advertised in the programming books!
>
> Sorry for this little rant but this bug really made me crazy.

Pardon me. Are you talking about a bug you introduced earlier in your own series that hasn't been applied (and you very well know will not be until 1.5.4 is out, now we are deep in -rc cycle)?

If so, you did a great disservice to me by sounding as if you are blaming somebody else's existing bug. I wasted some time hunting for a non-existent bug in the code that is being readied for 1.5.4 final for quite some time, in order to pick only the relevant "fix" from your patch.

It turns out, luckily, existing code did not have such a bug. What a relief for the maintainer in bugfix-only freeze mode.

Next time around, please mark the patch on the Subject: line to be squashed to your earlier [PATCH 5/6] before [PATCH 6/6]. That will also solve the bisectability problem.

Anyway, thanks. I was planning to queue the series in 'pu' or 'next' after tagging -rc3, so not be silent and giving a proper fix was the right thing to do. My above rant is just about the presentation.

Marco Costalba· Jan 12, 2008, 07:06 UTC · re: Junio C Hamano · lore

Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value

On Jan 12, 2008 1:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Next time around, please mark the patch on the Subject: line to
> be squashed to your earlier [PATCH 5/6] before [PATCH 6/6].
>

Very sorry for wasting your time I should have been more clear that it was a bug in the new series. And of course this series is not to be applied to stable git.

The only two points in the current code in master that I would like to report to you are a _possible_ missing inflateEnd() before a new inflateInit(), but I am not confident with that part of code to judge if is a bug or not, anyway that's the _possible_ diff.

Show changes to 2 files +2 −0

http-push.c, http-walker.c

diff --git a/http-push.c b/http-push.c
index 55d0c94..e0a4cc6 100644
--- a/http-push.c
+++ b/http-push.c
@@ -307,6 +307,7 @@ static void start_fetch_loose(struct
transfer_request *request)
 	/* Reset inflate/SHA1 if there was an error reading the previous temp
 	   file; also rewind to the beginning of the local file. */
 	if (prev_read == -1) {
+		inflateEnd(&request->stream);
 		memset(&request->stream, 0, sizeof(request->stream));
 		inflateInit(&request->stream);
 		SHA1_Init(&request->c);
diff --git a/http-walker.c b/http-walker.c
index 2c37868..a18067c 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -182,6 +182,7 @@ static void start_object_request(struct walker *walker,
 	/* Reset inflate/SHA1 if there was an error reading the previous temp
 	   file; also rewind to the beginning of the local file. */
 	if (prev_read == -1) {
+		inflateEnd(&obj_req->stream);
 		memset(&obj_req->stream, 0, sizeof(obj_req->stream));
 		inflateInit(&obj_req->stream);
 		SHA1_Init(&obj_req->c);



I have not created a proper patch becuase I don't know if the missing
inflateEnd(), it is a bug or not. The above diff it's just a way to
point you quickly and hopefully clearly to the interested code .


Sorry again for the trouble I had caused to you. For sure I will be
much more careful in the future to be clear in the subjects. And also
sorry for my rant but it was very late and I was tired after fighting
with that _my_ bug.

Marco
Junio C Hamano· Jan 12, 2008, 07:31 UTC · re: Marco Costalba · lore

Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value

"Marco Costalba" <mcostalba@gmail.com> writes:
Show 18 quoted lines
> The only two points in the current code in master that I would like to
> report to you are a _possible_ missing inflateEnd() before a new
> inflateInit(), but I am not confident with that part of code to judge
> if is a bug or not, anyway that's the _possible_ diff.
>
> diff --git a/http-push.c b/http-push.c
> index 55d0c94..e0a4cc6 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -307,6 +307,7 @@ static void start_fetch_loose(struct
> transfer_request *request)
>  	/* Reset inflate/SHA1 if there was an error reading the previous temp
>  	   file; also rewind to the beginning of the local file. */
>  	if (prev_read == -1) {
> +		inflateEnd(&request->stream);
>  		memset(&request->stream, 0, sizeof(request->stream));
>  		inflateInit(&request->stream);
>  		SHA1_Init(&request->c);

I think this could leak if request->stream already had something in it, but I do not see anything that is done to it after it is initialized and the code reaches to this point.

In fact, I suspect that it would make more sense to remove the earlier memset() that clears request->stream and inflateInit(), and moving the memset() and inflateInit() we see above out of that if clause (before checking prev_read).

The same comment applies to the other hunk.

By the way, I was looking at the earlier two series from you (compress and decompress), and noticed some of them were corrupt with linewrap. As I think they are good clean-up patches, I'd like to apply them as one of the first series post 1.5.4. As such, this request is not urgent at all, but please resend with a clean-up when 'master'/'next' reopens.

Thanks.
Marco Costalba· Jan 12, 2008, 07:42 UTC · re: Junio C Hamano · lore

Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value

On Jan 12, 2008 8:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
>
> By the way, I was looking at the earlier two series from you
> (compress and decompress), and noticed some of them were corrupt
> with linewrap.  As I think they are good clean-up patches, I'd
> like to apply them as one of the first series post 1.5.4.  As
> such, this request is not urgent at all, but please resend with
> a clean-up when 'master'/'next' reopens.
>
Sure.

Do you prefer patches differently organized or I can keep the same patch contents (of course with squashing the bug fixes in) ?

Marco
Junio C Hamano· Jan 12, 2008, 18:44 UTC · re: Marco Costalba · lore

Re: [PATCH decompress BUG] Fix decompress_next_from() wrong argument value

"Marco Costalba" <mcostalba@gmail.com> writes:
> Do you prefer patches differently organized or I can keep the same
> patch contents (of course with squashing the bug fixes in) ?

My impression was that the organization was good (addition of the helpers, and then conversion to existing code to use the helper piece-by-piece), even though I admit that I did not look at them very deeply.

← back to recent threads