Re: [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings

[Pedro Alves <palves@redhat.com>]

On 10/18/2013 03:39 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> These could be fixed by either reducing the length specified
>> in the format string, or, by increasing the buffers.  Either
>> such change would be obvious from a coding perspective.  But
>> the part that requires a rationale, is, that one that justifies
>> the taken approach.  That will be governed what the actual lengths
>> of these fields on the kernel side.  E.g.:
>>
>>       /* sizeof (cmd) should be greater or equal to TASK_COMM_LEN (in
>> 	 include/linux/sched.h in the Linux kernel sources) plus two
>> 	 (for the brackets).  */
>>       char cmd[32];
>>       PID_T stat_pid;
>>       int items_read = fscanf (fp, "%lld %32s", &stat_pid, cmd);
>>
>> Did you check the value of TASK_COMM_LEN ? (I haven't).
>>
>> Same for the other fields.
> 
> Ok, I think I have now checked *everything* :)
> 
> In the first hunk, the maximum size is 16 (including the terminator).
> Add two for the brackets makes 18, so 32 is big enough.  I don't know
> if there would be any benefit to reducing the buffer here to 18

I think it'd be beneficial for readability.  I'd prefer making it 18 then.

> (does it make a difference if things are declared to be a power of two in
> size?)

I don't think so.  In any case, this code is not really a hot path
or that size sensitive.

> For the second hunk I caused all the unused variables to be skipped,
> which took care of the buffer "extra" being too small.  I also reduced
> the size specifiers of local_address and remote_address from 33 to 32.
> This is the correct size (for IPv6) but was not causing an overflow as
> NI_MAXHOST is 1025 on my machine.  I added a compile-time check for
> this, but don't know if this is overkill.

That's fine.

> 
> In the third hunk, the dependencies field could be arbitrarily long,
> so I've rewritten it using strtok.  While doing this I noticed that
> size (an unsigned int) is parsed as "%d" but printed as "%u" so I
> changed the parsing to "%u".  I left untouched the funky indentation
> of the lines following the buffer_xml_printf but could change them
> to something else if required.
> 
> Is this ok?
> 

Yes, this looks excellent!  Thanks a lot for doing all this.


>  	      result = sscanf (buf,
> -			       "%d: %33[0-9A-F]:%X %33[0-9A-F]:%X %X %X:%X %X:%lX %X %d %d %lu %512s\n",
> -			       &sl,
> +			       "%*d: %32[0-9A-F]:%X %32[0-9A-F]:%X %X %*X:%*X %*X:%*X %*X %d %*d %*u %*s\n",
> +

Spurious newline?

>  			       local_address, &local_port,

-- 
Pedro Alves