[pve-devel] [PATCH] fix read after string end

[Thomas Lamprecht]

On 6/18/19 2:59 PM, Dominik Csapak wrote:
> On 6/18/19 2:37 PM, Thomas Lamprecht wrote:
>> On 6/18/19 2:24 PM, Dominik Csapak wrote:
>>> outs is not a zero-terminated string but has its length given by
>>> outslen, so use that (with a maximum of the size of msg)
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>>> ---
>>>   RADOS.xs | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/RADOS.xs b/RADOS.xs
>>> index f3f5516..7eca024 100644
>>> --- a/RADOS.xs
>>> +++ b/RADOS.xs
>>> @@ -131,8 +131,14 @@ CODE:
>>>         if (ret < 0) {
>>>           char msg[4096];
>>> -        snprintf(msg, sizeof(msg), "mon_command failed - %s\n", outs);
>>> +        if (outslen > sizeof(msg)) {
>>> +            outslen = sizeof(msg);
>>> +        }
>>
>> we always have some static string part which already consumes from
>> msg available space, so above is wrong? Why not just omit it, snprintf
>> will never print more than sizeof(msg) anyway?
> 
> outslen is size_t but we need an int for the field precision width
> so we have to cast but do not want to get some negative value in case
> it is too big, i just used the msg size as an upper limit
> we never write more than sizeof(msg)

a bit of a strange cast..

glibc just omits the precission if it's negative:
https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/vfprintf-internal.c;h=ead2b04cb96b0346dde876e5d41418fae2cc8b0d;hb=HEAD#l1554

Also from the man page (from parts you left out in your quote below
as irrelevant ;)
> A negative precision is taken as if the  precision  were omitted.

So that's also documented behavior, so I'd just pass it along, as
above looks like a bug (missed static length from "mon_command fa.."
and is a bit confusing, to me at least.

> 
> better would be to subtract the size of "mon_command failed - \n"
> but i did not want to copy that string, and some kind of define
> or const char * seemed a little much for this (the error
> messages are not that big in most cases anyway)
> 
>>
>>> +        snprintf(msg, sizeof(msg), "mon_command failed - %.*s\n", (int)outslen, outs);
>>
>> so, .* is using the field width functionallity of the *printf methods,
>> man snprintfs tells me:
>>
>>> Field width
>>> ... In no case does a nonexistent or small field width cause truncation of a field; if the result of  a  conversion  is
>>> wider than the field width, the field is expanded to contain the conversion result.
>>
>> So is this actually working?
> 
> this is not the field width, but the field precision
> man snprintf:
> 
> Precision
>  An  optional  precision, in the form of a period ('.')  followed by an
>  optional decimal digit string. [...] This gives the [...]
>  maximum  number of characters to be printed from a
>  string for s and S conversions.
> 
> (irrelevant parts omitted)
> 
> so it is the maximum number of characters for the string

OK, thanks for clarifying, maybe note something like that somewhere
in the vicinity of such a commit the next time :)

> 
>>
>>>           rados_buffer_free(outs);
>>> +        if (outbuf != NULL) {
>>> +            rados_buffer_free(outbuf);
>>> +        }
>>>           die(msg);
>>>       }
>>>  
>>