[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer
Date: Tue, 12 Jan 2016 17:52:38 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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.


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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]