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

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

[Marco Costalba]

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(-)

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

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

[Junio C Hamano]

"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.

> 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.

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

[Marco Costalba]

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.

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

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

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> 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.

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

[Marco Costalba]

On Jan 12, 2008 8:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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

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

[Junio C Hamano]

"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.