[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFi
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path |
Date: |
Mon, 23 Jul 2012 17:01:21 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.0.97 (gnu/linux) |
Laszlo Ersek <address@hidden> writes:
> On 07/23/12 14:46, Markus Armbruster wrote:
>> Laszlo Ersek <address@hidden> writes:
>>
>>> Signed-off-by: Laszlo Ersek <address@hidden>
>>> ---
>>> 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.
> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/snprintf.html#tag_16_159_04>)
>
> 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?
If I read your code correctly, qdev_get_fw_dev_path_helper() bails out
after the first snprintf() that goes beyond the buffer size. When that
happens before the job's done, the return value is less than the length
of the full path.
static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
{
int l = 0;
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; <--- bail out early
+ }
+
if (dev->parent_bus->info->get_fw_dev_path) {
d = dev->parent_bus->info->get_fw_dev_path(dev);
l += snprintf(p + l, size - l, "%s", d);
qemu_free(d);
} else {
l += snprintf(p + l, size - l, "%s", dev->info->name);
}
+
+ if (l >= size) {
+ return l; <--- bail out early
+ }
}
l += snprintf(p + l , size - l, "/");
return l;
}
>>> @@ -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 :)
Only marginally harder than handling the failure :)
- [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path, Laszlo Ersek, 2012/07/23
- Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path, Peter Maydell, 2012/07/23
- Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path, Markus Armbruster, 2012/07/23
- Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path, Laszlo Ersek, 2012/07/23
- Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path, Laszlo Ersek, 2012/07/23
- Re: [Qemu-devel] [PATCH] check for available room when formatting OpenFirmware device path, Markus Armbruster, 2012/07/23
- [Qemu-devel] [PATCH v2 0/2] assorted device path formatting cleanups, Laszlo Ersek, 2012/07/24
- [Qemu-devel] [PATCH v2 1/2] accomodate OpenFirmware device paths in sufficient storage, Laszlo Ersek, 2012/07/24
- Re: [Qemu-devel] [PATCH v2 1/2] accomodate OpenFirmware device paths in sufficient storage, Peter Maydell, 2012/07/25
- Re: [Qemu-devel] [PATCH v2 1/2] accomodate OpenFirmware device paths in sufficient storage, Markus Armbruster, 2012/07/25
- [Qemu-devel] [PATCH v2 2/2] get_fw_dev_path() impls should allocate memory with glib functions, Laszlo Ersek, 2012/07/24