Re: [PATCH 5/5] streaming: simplify attaching a filter

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2014, 00:02 UTC
Message-ID: <xmqq8ut8as1f.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqd2ikasbe.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 12 quoted lines

> John Keeping <john@keeping.me.uk> writes:
>
>> We are guaranteed that 'nst' is non-null because it is allocated with
>> xmalloc(), and in fact we rely on this three lines later by
>> unconditionally dereferencing it.
>
> The intent of the original code is for attach_stream_filter() to
> detect an error condition and return NULL, in which case it closes
> the istream it allocated and signal error to the caller, I think,
> and falling thru to use st->anything and return st when that happens
> is *not* a guarantee that a-s-f will not detect an error ever, but
> rather is a bug in the error codepath.

In other words, something like this instead.

-- >8 --
Subject: [PATCH] open_istream(): do not dereference NULL in the error case

When stream-filter cannot be attached, it is expected to return NULL, and we should close the stream we opened and signal an error by returning NULL ourselves from this function.

However, we attempted to dereference that NULL pointer between the point we detected the error and returned from the function.

Brought-to-attention-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 streaming.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/streaming.c b/streaming.c
index d7c9f32..2ff036a 100644
--- a/streaming.c
+++ b/streaming.c
@@ -152,8 +152,10 @@ struct git_istream *open_istream(const unsigned char *sha1,
 	if (filter) {
 		/* Add "&& !is_null_stream_filter(filter)" for performance */
 		struct git_istream *nst = attach_stream_filter(st, filter);
-		if (!nst)
+		if (!nst) {
 			close_istream(st);
+			return NULL;
+		}
 		st = nst;
 	}

-- 
1.9.0-279-gdc9e3eb