| Message ID | 1470393800-7882-1-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ping? (Now that 2.7 is out it would be nice to get rid of the clang warnings cluttering up my build logs :-)) thanks -- PMM On 5 August 2016 at 11:43, Peter Maydell <peter.maydell@linaro.org> wrote: > Some tests use the qtest protocol "memset" command with a zero > size, expecting it to do nothing. However in the current code this > will result in calling memset() with a NULL pointer, which is > undefined behaviour. Detect and specially handle zero sizes to > avoid this. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Looking at the code for the other commands that take a size > ('read', 'write', 'b64read' and 'b64write' they all assume a > non-zero size. I've left those alone though, somebody else can > make them do nothing on zero size if they feel it's important.) > > qtest.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/qtest.c b/qtest.c > index da4826c..ce4c6db 100644 > --- a/qtest.c > +++ b/qtest.c > @@ -133,6 +133,7 @@ static bool qtest_opened; > * < OK > * > * ADDR, SIZE, VALUE are all integers parsed with strtoul() with a base of 0. > + * For 'memset' a zero size is permitted and does nothing. > * > * DATA is an arbitrarily long hex number prefixed with '0x'. If it's smaller > * than the expected size, the value will be zero filled at the end of the data > @@ -493,10 +494,12 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) > len = strtoull(words[2], NULL, 0); > pattern = strtoull(words[3], NULL, 0); > > - data = g_malloc(len); > - memset(data, pattern, len); > - cpu_physical_memory_write(addr, data, len); > - g_free(data); > + if (len) { > + data = g_malloc(len); > + memset(data, pattern, len); > + cpu_physical_memory_write(addr, data, len); > + g_free(data); > + } > > qtest_send_prefix(chr); > qtest_send(chr, "OK\n"); > -- > 2.7.4
On 08/05/2016 05:43 AM, Peter Maydell wrote: > Some tests use the qtest protocol "memset" command with a zero > size, expecting it to do nothing. However in the current code this > will result in calling memset() with a NULL pointer, which is > undefined behaviour. Detect and specially handle zero sizes to > avoid this. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Looking at the code for the other commands that take a size > ('read', 'write', 'b64read' and 'b64write' they all assume a > non-zero size. I've left those alone though, somebody else can > make them do nothing on zero size if they feel it's important.) I obviously missed reviewing this in time for 2.7, but looks reasonable to me. Reviewed-by: Eric Blake <eblake@redhat.com>
On 8 September 2016 at 15:37, Eric Blake <eblake@redhat.com> wrote: > On 08/05/2016 05:43 AM, Peter Maydell wrote: >> Some tests use the qtest protocol "memset" command with a zero >> size, expecting it to do nothing. However in the current code this >> will result in calling memset() with a NULL pointer, which is >> undefined behaviour. Detect and specially handle zero sizes to >> avoid this. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> Looking at the code for the other commands that take a size >> ('read', 'write', 'b64read' and 'b64write' they all assume a >> non-zero size. I've left those alone though, somebody else can >> make them do nothing on zero size if they feel it's important.) > > I obviously missed reviewing this in time for 2.7, but looks reasonable > to me. > > Reviewed-by: Eric Blake <eblake@redhat.com> Applied to master, thanks. -- PMM
diff --git a/qtest.c b/qtest.c index da4826c..ce4c6db 100644 --- a/qtest.c +++ b/qtest.c @@ -133,6 +133,7 @@ static bool qtest_opened; * < OK * * ADDR, SIZE, VALUE are all integers parsed with strtoul() with a base of 0. + * For 'memset' a zero size is permitted and does nothing. * * DATA is an arbitrarily long hex number prefixed with '0x'. If it's smaller * than the expected size, the value will be zero filled at the end of the data @@ -493,10 +494,12 @@ static void qtest_process_command(CharDriverState *chr, gchar **words) len = strtoull(words[2], NULL, 0); pattern = strtoull(words[3], NULL, 0); - data = g_malloc(len); - memset(data, pattern, len); - cpu_physical_memory_write(addr, data, len); - g_free(data); + if (len) { + data = g_malloc(len); + memset(data, pattern, len); + cpu_physical_memory_write(addr, data, len); + g_free(data); + } qtest_send_prefix(chr); qtest_send(chr, "OK\n");
Some tests use the qtest protocol "memset" command with a zero size, expecting it to do nothing. However in the current code this will result in calling memset() with a NULL pointer, which is undefined behaviour. Detect and specially handle zero sizes to avoid this. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Looking at the code for the other commands that take a size ('read', 'write', 'b64read' and 'b64write' they all assume a non-zero size. I've left those alone though, somebody else can make them do nothing on zero size if they feel it's important.) qtest.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)