Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer

[Markus Armbruster]

Wolfgang Bumiller <address@hidden> writes:

>> On January 12, 2016 at 5:00 PM Markus Armbruster <address@hidden> wrote:
>>         separator = strchr(keys, '-');
>>         keyname_len = separator ? separator - keys : strlen(keys);
>>         pstrcpy(keyname_buf, sizeof(keyname_buf), keys);
>>         [...]
>>         keyname_buf[keyname_len] = 0;
>> 
>> This is stupid.  If we already know how many characters we need, we
>> should copy exactly those:
>
> I mentioned that and didn't touch it because the same holds for the
> copying of the word "less" below and should IMO be in a separate
> cleanup patch together with that one.

Stupidest way I can think of:

>>         separator = strchr(keys, '-');
>>         keyname_len = separator ? separator - keys : strlen(keys);
>>         if (keyname_len >= sizeof(keyname_buf))
>>             goto err_out;
>>         memcpy(keyname_buf, keyname_len, keys);
>>         keyname_buf[keyname_len] = 0;

           if (!strcmp(keyname_buf, "<")) {
               strcpy(keyname_buf, "less");
           }

>> But I'd simply dispense with the static buffer, and do something like
>> 
>>         separator = strchr(keys, '-');
>>         key = separator ? g_strndup(keys, separator - keys) : g_strdup(keys);
>>         [...]
>>         g_free(key);
>> 
>> This is advice, not recommendation.
>
> Sure, also a good alternative.
>
>> (...)
>> 
>> Will you prepare a revised patch?
>
> Can do that tomorrow, but which option is the preferred one? If "%.*s" works
> everywhere then changing index_from_key() and using "%.*s" would be the most
> optimal I think.
>
> I don't want to bounce 5 more versions back and forth of something that's
> supposed to be rather trivial.

Understandable.

If your patch works and is simple, I won't ask you to redo it using
another method just because I might like that better.

Your previous patch almost qualifies: it's simple, it fixes the bug, but
it regresses the error message a bit.  I pointed out how to avoid the
latter, and I asked you to either add comments explaining why truncation
works (even though it's preexisting), or to redo the thing in a more
obvious way (your choice).  I'm pretty sure your next patch will be fine
or very close.