From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59546) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1StIME-0001Gf-82 for qemu-devel@nongnu.org; Mon, 23 Jul 2012 09:07:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1StIM8-0006Sp-9h for qemu-devel@nongnu.org; Mon, 23 Jul 2012 09:07:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22022) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1StIM8-0006Sl-1j for qemu-devel@nongnu.org; Mon, 23 Jul 2012 09:07:24 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q6ND7Njs001996 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 23 Jul 2012 09:07:23 -0400 Message-ID: <500D4CD4.5030706@redhat.com> Date: Mon, 23 Jul 2012 15:08:36 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1343043213-9997-1-git-send-email-lersek@redhat.com> <874noyhcfz.fsf@blackfin.pond.sub.org> In-Reply-To: <874noyhcfz.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org On 07/23/12 14:46, Markus Armbruster wrote: > Laszlo Ersek writes: > >> Signed-off-by: Laszlo Ersek >> --- >> hw/qdev.c | 14 +++++++++++++- >> vl.c | 7 ++++++- >> 2 files changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/hw/qdev.c b/hw/qdev.c >> index af54467..f1e83a4 100644 >> --- a/hw/qdev.c >> +++ b/hw/qdev.c >> @@ -502,6 +502,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size) >> if (dev && dev->parent_bus) { >> char *d; >> l = qdev_get_fw_dev_path_helper(dev->parent_bus->parent, p, size); >> + if (l >= size) { >> + return l; >> + } >> + >> d = bus_get_fw_dev_path(dev->parent_bus, dev); >> if (d) { >> l += snprintf(p + l, size - l, "%s", d); >> @@ -509,6 +513,10 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size) >> } else { >> l += snprintf(p + l, size - l, "%s", object_get_typename(OBJECT(dev))); >> } >> + >> + if (l >= size) { >> + return l; >> + } >> } >> l += snprintf(p + l , size - l, "/"); >> > > If the return value is less than the size argument, it's the length of > the string written into p[]. Else, it means p[] has insufficient > space. Yes. (snprintf() returns the number of bytes it would store, excluding the terminating NUL, had there been enough room. ) Did I make a mistake? Supposing snprintf() encounters no error, it returns a positive value P in the above. P = snprintf(..., size - l0, ...) l1 = l0 + P; l1 >= size <-> l0 + P >= size <-> P >= size - l0 The return value of qdev_get_fw_dev_path_helper() comes from another invocation of itself, or from the trailing snprintf(), so it behaves like snprintf. Or what do you have in mind? > >> @@ -520,8 +528,12 @@ char* qdev_get_fw_dev_path(DeviceState *dev) >> char path[128]; >> int l; >> >> - l = qdev_get_fw_dev_path_helper(dev, path, 128); >> + l = qdev_get_fw_dev_path_helper(dev, path, sizeof(path)); >> >> + assert(l > 0); >> + if (l >= sizeof(path)) { >> + return NULL; >> + } > > Failure mode could be avoided with the common technique: make > qdev_get_fw_dev_path_helper() return the true length. If it fit into > path[], return strdup(path). Else, allocate a suitable buffer and try > again. > > What do you think? You're right of course, but I didn't have (or wanted to spend) the time to change the code that much (and to test it...), especially because it would have been fixed already if it had caused actual problems. I think the patch could work as a "last resort", small improvement, but I don't mind if someone posts a better fix :) Laszlo