This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings
- From: Pedro Alves <palves at redhat dot com>
- To: gdb-patches at sourceware dot org, dcb314 at hotmail dot com
- Date: Fri, 18 Oct 2013 17:38:28 +0100
- Subject: Re: [PATCH v2][PR gdb/16013] Fix off-by-one errors in *scanf format strings
- Authentication-results: sourceware.org; auth=none
- References: <20131014105252 dot GA5262 at blade dot nx> <525BD49B dot 4080700 at redhat dot com> <20131018143903 dot GA14055 at blade dot nx>
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