[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 5/5] migration: Use strnlen() for fixed-size
Re: [Qemu-block] [PATCH v3 5/5] migration: Use strnlen() for fixed-size string
Tue, 18 Dec 2018 13:33:43 -0600
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1
On 12/18/18 11:51 AM, Philippe Mathieu-Daudé wrote:
GCC 8 introduced the -Wstringop-overflow, which detect buffer overflow
by string-modifying functions declared in <string.h>, such strncpy(),
used in global_state_store_running().
Since the global_state.runstate does not necessarily contains a
terminating NUL character, We had to use the QEMU_NONSTRING attribute.
The GCC manual says about the nonstring attribute:
However, when the array is declared with the attribute the call to
strlen is diagnosed because when the array doesn’t contain a
NUL-terminated string the call is undefined. [...]
In addition, calling strnlen and strndup with such arrays is safe
provided a suitable bound is specified, and not diagnosed.
GCC indeed found an incorrect use of strlen(), because this array
is loaded by VMSTATE_BUFFER(runstate, GlobalState) then parsed
using qapi_enum_parse which does not get the buffer length.
Use strnlen() which returns sizeof(s->runstate) if the array is not
qemu/migration/global_state.c: In function 'global_state_pre_save':
qemu/migration/global_state.c:109:15: error: 'strlen' argument 1 declared
attribute 'nonstring' [-Werror=stringop-overflow=]
s->size = strlen((char *)s->runstate) + 1;
qemu/migration/global_state.c:24:13: note: argument 'runstate' declared here
uint8_t runstate QEMU_NONSTRING;
cc1: all warnings being treated as errors
make: *** [qemu/rules.mak:69: migration/global_state.o] Error 1
Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
migration/global_state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/global_state.c b/migration/global_state.c
index 6e19333422..c19030ef62 100644
@@ -106,7 +106,7 @@ static int global_state_pre_save(void *opaque)
GlobalState *s = opaque;
- s->size = strlen((char *)s->runstate) + 1;
The old code sets s->size to the string length + space for the NUL byte
(by assuming that a NUL byte was present), and accidentally sets it
beyond the s->runstate array if there was no NUL byte (our existing
runstate names are shorter than 100 bytes, so this could only happen on
a malicious stream).
+ s->size = strnlen((char *)s->runstate, sizeof(s->runstate)) + 1;
The new code can still end up setting s->size beyond the array. Is that
intended, or would it be better to use strnlen(s->runstate,
sizeof(s->runstate) - 1) + 1?
Also, as I argued on 4/5, why isn't this squashed in with the patch that
marks the field NONSTRING?
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Qemu-block] [PATCH v3 4/5] migration: Use QEMU_NONSTRING for non NUL-terminated arrays, Philippe Mathieu-Daudé, 2018/12/18
[Qemu-block] [PATCH v3 5/5] migration: Use strnlen() for fixed-size string, Philippe Mathieu-Daudé, 2018/12/18
Re: [Qemu-block] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation, Philippe Mathieu-Daudé, 2018/12/18
Re: [Qemu-block] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation, Michael S. Tsirkin, 2018/12/18
Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/5] Fix strncpy() warnings for GCC8 new -Wstringop-truncation, no-reply, 2018/12/24
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays, (continued)