Re: [PATCH 8/6 v2] receive-pack: Send internal errors over side-band #2

From: Shawn O. Pearce <spearce@spearce.org>
Date: Feb 11, 2010, 15:05 UTC
Message-ID: <20100211150547.GL2747@spearce.org>
In-Reply-To: <4B73C0FF.5020503@viscovery.net>

Johannes Sixt <j.sixt@viscovery.net> wrote:

Show 14 quoted lines

> Shawn O. Pearce schrieb:
>> +static void report_message(const char *prefix, const char *err, va_list params)
>> +{
>> +	int sz = strlen(prefix);
>> +	char msg[4096];
>> +
>> +	strncpy(msg, prefix, sz);
>> +	sz += vsnprintf(msg + sz, sizeof(msg) - sz, err, params);
>> +	if (sz > (sizeof(msg) - 1))
>> +		sz = sizeof(msg) - 1;
>> +	msg[sz++] = '\n';
>
> Sorry, still no joy - the terminating NUL is missing (I should have  
> noticed this in your v1 already).

Why is it necessary?

Once the msg buffer is prepared, its written using its length, sz, not its NUL termination status. Neither send_sideband() nor xwrite() care about NUL termination.

The only reason to put a NUL onto this buffer is so you can do "p msg" within GDB and get a useful result. We don't typically do this in these contexts.

Or did my MTA inject additional C code I didn't write?

Show 7 quoted lines

> I suggest to forgo the length check for 
> simplicity because this function is only called with data that is already 
> guaranteed to be less than 1000 bytes, i.e.:
>
> 	strncpy(msg, prefix, sz);
> 	/* data is guaranteed to fit due to packet length limit in  
> read_head_info() */

Yea, that's true now. We probably could have used 1200 bytes for the msg buffer, rather than 4096. Even the huge warnings about current branch behavior are broken up into one string per line.

-- 
Shawn.