Re: [RFC PATCH v4 03/26] pkt-line: Make packet_read_line easier to debug

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 29, 2009, 15:11 UTC
Message-ID: <20091029151152.GX10505@spearce.org>
In-Reply-To: <7vhbtidgmp.fsf@alter.siamese.dyndns.org>

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

Show 22 quoted lines

> "Shawn O. Pearce" <spearce@spearce.org> writes:
> > diff --git a/pkt-line.c b/pkt-line.c
> > index bd603f8..893dd3c 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -124,12 +124,14 @@ static int packet_length(const char *linelen)
> >  int packet_read_line(int fd, char *buffer, unsigned size)
> >  {
> >  	int len;
> > -	char linelen[4];
> > +	char linelen[5];
> >  
> >  	safe_read(fd, linelen, 4);
> >  	len = packet_length(linelen);
> > -	if (len < 0)
> > -		die("protocol error: bad line length character");
> > +	if (len < 0) {
> > +		linelen[4] = '\0';
> > +		die("protocol error: bad line length character: %s", linelen);
> > +	}
> 
> Since this is not called recursively, you can make linelen[] static

Sure. But it wasn't static before. It was stack allocated. Its a 5 byte stack allocation. Its not a lot of data to push onto the stack, why does it suddenly matter that we make it static and charge everyone for its memory instead of just those who really need it?

> and do
> without the NUL assignment; safe_read() won't read beyond 4 bytes anyway.

The NUL assignment isn't about safe_read(), its about making the block of 4 bytes read a proper C-style string so we can safely pass it down into the snprintf that is within die().

-- 
Shawn.