[Qemu-devel] [PATCH 0/2] qstring: Safer qstring_from_substr()

[Qemu-devel] [PATCH 0/2] qstring: Safer qstring_from_substr()

[Markus Armbruster]

This is based on liujunjie's "[PATCH] qstring: Fix
qstring_from_substr() not to provoke int overflow".  I intend to get
that one into 3.0 as a bug fix.  I figure PATCH 1 should go into 3.0
as a safety measure.  I'm not sure about PATCH 2.  Please review.

Based-on: 20180724134339.17832-1-liujunjie23@huawei.com

Markus Armbruster (2):
  qstring: Assert size calculations don't overflow
  qstring: Move qstring_from_substr()'s @end one to the right

 block/blkdebug.c      |  2 +-
 block/blkverify.c     |  2 +-
 block/nbd.c           |  2 +-
 qobject/qstring.c     | 10 +++++++---
 tests/check-qobject.c |  2 +-
 tests/check-qstring.c |  2 +-
 6 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] qstring: Assert size calculations don't overflow

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qobject/qstring.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qobject/qstring.c b/qobject/qstring.c
index 18b8eb82f8..7990569c5a 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -41,17 +41,19 @@ QString *qstring_from_substr(const char *str, size_t start, size_t end)
 {
     QString *qstring;
 
+    assert(start <= end + 1);
+
     qstring = g_malloc(sizeof(*qstring));
     qobject_init(QOBJECT(qstring), QTYPE_QSTRING);
 
     qstring->length = end - start + 1;
     qstring->capacity = qstring->length;
 
+    assert(qstring->capacity < SIZE_MAX);
     qstring->string = g_malloc(qstring->capacity + 1);
     memcpy(qstring->string, str + start, qstring->length);
     qstring->string[qstring->length] = 0;
 
-
     return qstring;
 }
 
@@ -68,7 +70,9 @@ QString *qstring_from_str(const char *str)
 static void capacity_increase(QString *qstring, size_t len)
 {
     if (qstring->capacity < (qstring->length + len)) {
+        assert(len <= SIZE_MAX - qstring->capacity);
         qstring->capacity += len;
+        assert(qstring->capacity + len <= SIZE_MAX / 2);
         qstring->capacity *= 2; /* use exponential growth */
 
         qstring->string = g_realloc(qstring->string, qstring->capacity + 1);
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] qstring: Move qstring_from_substr()'s @end one to the right

[Markus Armbruster]

qstring_from_substr() takes the index of the substring's first and
last character.  qstring_from_substr(s, 0, SIZE_MAX) denotes an empty
substring.  Awkward.

Shift the end index one to the right.  This simplifies both
qstring_from_substr() and its callers.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/blkdebug.c      | 2 +-
 block/blkverify.c     | 2 +-
 block/nbd.c           | 2 +-
 qobject/qstring.c     | 6 +++---
 tests/check-qobject.c | 2 +-
 tests/check-qstring.c | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 0457bf5b66..0759452925 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -305,7 +305,7 @@ static void blkdebug_parse_filename(const char *filename, QDict *options,
 
     if (c != filename) {
         QString *config_path;
-        config_path = qstring_from_substr(filename, 0, c - filename - 1);
+        config_path = qstring_from_substr(filename, 0, c - filename);
         qdict_put(options, "config", config_path);
     }
 
diff --git a/block/blkverify.c b/block/blkverify.c
index da97ee5927..89bf4386e3 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -80,7 +80,7 @@ static void blkverify_parse_filename(const char *filename, QDict *options,
     }
 
     /* TODO Implement option pass-through and set raw.filename here */
-    raw_path = qstring_from_substr(filename, 0, c - filename - 1);
+    raw_path = qstring_from_substr(filename, 0, c - filename);
     qdict_put(options, "x-raw", raw_path);
 
     /* TODO Allow multi-level nesting and set file.filename here */
diff --git a/block/nbd.c b/block/nbd.c
index b198ad775f..e87699fb73 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -109,7 +109,7 @@ static int nbd_parse_uri(const char *filename, QDict *options)
         /* strip braces from literal IPv6 address */
         if (uri->server[0] == '[') {
             host = qstring_from_substr(uri->server, 1,
-                                       strlen(uri->server) - 2);
+                                       strlen(uri->server) - 1);
         } else {
             host = qstring_from_str(uri->server);
         }
diff --git a/qobject/qstring.c b/qobject/qstring.c
index 7990569c5a..6afb8b005c 100644
--- a/qobject/qstring.c
+++ b/qobject/qstring.c
@@ -41,12 +41,12 @@ QString *qstring_from_substr(const char *str, size_t start, size_t end)
 {
     QString *qstring;
 
-    assert(start <= end + 1);
+    assert(start <= end);
 
     qstring = g_malloc(sizeof(*qstring));
     qobject_init(QOBJECT(qstring), QTYPE_QSTRING);
 
-    qstring->length = end - start + 1;
+    qstring->length = end - start;
     qstring->capacity = qstring->length;
 
     assert(qstring->capacity < SIZE_MAX);
@@ -64,7 +64,7 @@ QString *qstring_from_substr(const char *str, size_t start, size_t end)
  */
 QString *qstring_from_str(const char *str)
 {
-    return qstring_from_substr(str, 0, strlen(str) - 1);
+    return qstring_from_substr(str, 0, strlen(str));
 }
 
 static void capacity_increase(QString *qstring, size_t len)
diff --git a/tests/check-qobject.c b/tests/check-qobject.c
index 16ccbde82c..593c3a0618 100644
--- a/tests/check-qobject.c
+++ b/tests/check-qobject.c
@@ -154,7 +154,7 @@ static void qobject_is_equal_string_test(void)
     str_case = qstring_from_str("Foo");
 
     /* Should yield "foo" */
-    str_built = qstring_from_substr("form", 0, 1);
+    str_built = qstring_from_substr("form", 0, 2);
     qstring_append_chr(str_built, 'o');
 
     check_unequal(str_base, str_whitespace_0, str_whitespace_1,
diff --git a/tests/check-qstring.c b/tests/check-qstring.c
index f11a7a8605..2d079921e3 100644
--- a/tests/check-qstring.c
+++ b/tests/check-qstring.c
@@ -66,7 +66,7 @@ static void qstring_from_substr_test(void)
 {
     QString *qs;
 
-    qs = qstring_from_substr("virtualization", 3, 9);
+    qs = qstring_from_substr("virtualization", 3, 10);
     g_assert(qs != NULL);
     g_assert(strcmp(qstring_get_str(qs), "tualiza") == 0);
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/2] qstring: Assert size calculations don't overflow

[Eric Blake]

On 07/26/2018 01:18 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qobject/qstring.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index 18b8eb82f8..7990569c5a 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -41,17 +41,19 @@ QString *qstring_from_substr(const char *str, size_t start, size_t end)
>   {
>       QString *qstring;
>   
> +    assert(start <= end + 1);

end + 1 can overflow size_t, but it is unsigned so well-defined, and the 
assert will trigger as desired.

> @@ -68,7 +70,9 @@ QString *qstring_from_str(const char *str)
>   static void capacity_increase(QString *qstring, size_t len)
>   {
>       if (qstring->capacity < (qstring->length + len)) {
> +        assert(len <= SIZE_MAX - qstring->capacity);
>           qstring->capacity += len;

You've asserted that this addition won't overflow...

> +        assert(qstring->capacity + len <= SIZE_MAX / 2);

...but now that qstring->capacity is larger, this could overflow. Do you 
really need the +len in here, given that...

>           qstring->capacity *= 2; /* use exponential growth */

...you are really only trying to prevent overflow of doubling 
qstring->capacity without adding yet another len in the mix?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/2] qstring: Move qstring_from_substr()'s @end one to the right

[Eric Blake]

On 07/26/2018 01:18 AM, Markus Armbruster wrote:
> qstring_from_substr() takes the index of the substring's first and
> last character.  qstring_from_substr(s, 0, SIZE_MAX) denotes an empty
> substring.  Awkward.
> 
> Shift the end index one to the right.  This simplifies both
> qstring_from_substr() and its callers.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Not strictly a bug fix, but found while fixing a bug, thus I'm okay if 
this makes it into 3.0 (your call as maintainer).

>   block/blkdebug.c      | 2 +-
>   block/blkverify.c     | 2 +-
>   block/nbd.c           | 2 +-
>   qobject/qstring.c     | 6 +++---
>   tests/check-qobject.c | 2 +-
>   tests/check-qstring.c | 2 +-
>   6 files changed, 8 insertions(+), 8 deletions(-)
> 

This does fix all users.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/2] qstring: Assert size calculations don't overflow

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/26/2018 01:18 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   qobject/qstring.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/qobject/qstring.c b/qobject/qstring.c
>> index 18b8eb82f8..7990569c5a 100644
>> --- a/qobject/qstring.c
>> +++ b/qobject/qstring.c
>> @@ -41,17 +41,19 @@ QString *qstring_from_substr(const char *str, size_t start, size_t end)
>>   {
>>       QString *qstring;
>>   +    assert(start <= end + 1);
>
> end + 1 can overflow size_t, but it is unsigned so well-defined, and
> the assert will trigger as desired.
>
>> @@ -68,7 +70,9 @@ QString *qstring_from_str(const char *str)
>>   static void capacity_increase(QString *qstring, size_t len)
>>   {
>>       if (qstring->capacity < (qstring->length + len)) {
>> +        assert(len <= SIZE_MAX - qstring->capacity);
>>           qstring->capacity += len;
>
> You've asserted that this addition won't overflow...
>
>> +        assert(qstring->capacity + len <= SIZE_MAX / 2);
>
> ...but now that qstring->capacity is larger, this could overflow. Do
> you really need the +len in here, given that...
>
>>           qstring->capacity *= 2; /* use exponential growth */
>
> ...you are really only trying to prevent overflow of doubling
> qstring->capacity without adding yet another len in the mix?

You're right, my assertion is broken.  v2 coming.