Re: [PATCH 1/5] avoid parse_sha1_header() accessing memory out of bound

> Liu Yubao <yubao.liu@gmail.com> wrote:
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 6c0e251..efe6967 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -1254,10 +1255,10 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>>  	/*
>>  	 * The type can be at most ten bytes (including the
>>  	 * terminating '\0' that we add), and is followed by
>> -	 * a space.
>> +	 * a space, at least one byte for size, and a '\0'.
>>  	 */
>>  	i = 0;
>> -	for (;;) {
>> +	while (hdr < hdr_end - 2) {
>>  		char c = *hdr++;
>>  		if (c == ' ')
>>  			break;
>> @@ -1265,6 +1266,8 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>>  		if (i >= sizeof(type))
>>  			return -1;
> 
> That first hunk I am citing is unnecessary, because of the lines
> right above.  All of the callers of this function pass in a buffer
> that is at least 32 bytes in size; this loop aborts if it does not
> find a ' ' within the first 10 bytes of the buffer.  We'll never
> access memory outside of the buffer during this loop.
> 
> So IMHO your first three hunks here aren't necessary.
> 

>> @@ -1275,7 +1278,7 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>>  	if (size > 9)
>>  		return -1;
>>  	if (size) {
>> -		for (;;) {
>> +		while (hdr < hdr_end - 1) {
>>  			unsigned long c = *hdr - '0';
>>  			if (c > 9)
>>  				break;
> 
> OK, there's no promise here that we don't roll off the buffer.
> 
> This can be fixed in the caller, ensuring we always have the '\0'
> at some point in the initial header buffer we were asked to parse:
>