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

[Dominik Csapak]

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)

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

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