From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32853) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJ2Bb-000623-MR for qemu-devel@nongnu.org; Tue, 12 Jan 2016 11:52:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJ2BX-0003vP-BE for qemu-devel@nongnu.org; Tue, 12 Jan 2016 11:52:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58396) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJ2BX-0003vL-2f for qemu-devel@nongnu.org; Tue, 12 Jan 2016 11:52:43 -0500 From: Markus Armbruster References: <20160108091949.GA14724@olga> <20160108130251.GA17847@olga> <20160108143831.GA7632@olga> <1907860725.4.388b58e8-5b06-4844-be0c-df2778eb46fb.open-xchange@webmail.proxmox.com> <56920EC7.6090109@msgid.tls.msk.ru> <20160111075914.GA28466@olga> <87wprfqj8t.fsf@blackfin.pond.sub.org> <1786291473.38.33445737-8c34-4632-b55f-f565652bb5d3.open-xchange@webmail.proxmox.com> <87h9iirdms.fsf@blackfin.pond.sub.org> <1388889179.80.33445737-8c34-4632-b55f-f565652bb5d3.open-xchange@webmail.proxmox.com> Date: Tue, 12 Jan 2016 17:52:38 +0100 In-Reply-To: <1388889179.80.33445737-8c34-4632-b55f-f565652bb5d3.open-xchange@webmail.proxmox.com> (Wolfgang Bumiller's message of "Tue, 12 Jan 2016 17:25:20 +0100 (CET)") Message-ID: <877fjepwo9.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] hmp: avoid redundant null termination of buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wolfgang Bumiller Cc: Ling Liu , Michael Tokarev , qemu-devel@nongnu.org, P J P Wolfgang Bumiller writes: >> On January 12, 2016 at 5:00 PM Markus Armbruster 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.