[PATCH 0/2] Fix segfault in qemu_rbd_parse_filename

[PATCH 0/2] Fix segfault in qemu_rbd_parse_filename

[Connor Kuehl]

Connor Kuehl (2):
  iotests/231: Update expected deprecation message
  block/rbd: Don't unescape in qemu_rbd_next_tok()

 block/rbd.c                | 3 ---
 tests/qemu-iotests/231     | 4 ++++
 tests/qemu-iotests/231.out | 7 ++++---
 3 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.30.2

[PATCH 1/2] iotests/231: Update expected deprecation message

[Connor Kuehl]

The deprecation message changed slightly at some point in the past but
the expected output wasn't updated along with it; causing it to fail.
Fix it, so it passes.

Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
---
 tests/qemu-iotests/231.out | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 579ba11c16..747dd221bb 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -1,9 +1,7 @@
 QA output created by 231
-qemu-img: RBD options encoded in the filename as keyvalue pairs is deprecated.  Future versions may cease to parse these options in the future.
+qemu-img: warning: RBD options encoded in the filename as keyvalue pairs is deprecated
 unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
 qemu-img: Could not open 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': error connecting: No such file or directory
 unable to get monitor info from DNS SRV with service name: ceph-mon
-no monitors specified to connect to.
 qemu-img: Could not open 'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}': error connecting: No such file or directory
 *** done
-- 
2.30.2

Re: [PATCH 1/2] iotests/231: Update expected deprecation message

[Max Reitz]

On 01.04.21 17:52, Connor Kuehl wrote:
> The deprecation message changed slightly at some point in the past but
> the expected output wasn't updated along with it; causing it to fail.
> Fix it, so it passes.
> 
> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> ---
>   tests/qemu-iotests/231.out | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

Uh, well, you know what, I can’t find any version where there was any 
other output.  Even back in 66e6a735e97450ac50fcaf40f78600c688534cae, 
where this test was introduced, I get this diff.

What’s going on there?

Max

Re: [PATCH 1/2] iotests/231: Update expected deprecation message

[Max Reitz]

On 01.04.21 18:52, Max Reitz wrote:
> On 01.04.21 17:52, Connor Kuehl wrote:
>> The deprecation message changed slightly at some point in the past but
>> the expected output wasn't updated along with it; causing it to fail.
>> Fix it, so it passes.
>>
>> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
>> ---
>>   tests/qemu-iotests/231.out | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> Uh, well, you know what, I can’t find any version where there was any 
> other output.  Even back in 66e6a735e97450ac50fcaf40f78600c688534cae, 
> where this test was introduced, I get this diff.
> 
> What’s going on there?

Okay.  So:

Jeff’s original patch[1] included the “Future versions may cease to 
parse...” part.  v1 of his subsequent pull request[2] did, too.  But 
v2[3] didn’t.  Looks like Markus made a comment on v4 of the patch, and 
then Jeff fixed up the patch in his branch, but didn’t change the test. 
  In any case it’s clear that the reference output was wrong all along.

About the “no monitors specified” part...  The only place where I can 
find “no monitors” is in Jeff’s patches to add this iotest.  I have no 
idea where that orignated from.

So:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[1]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00282.html

[2]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00307.html

[3]
https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00592.html

Re: [PATCH 1/2] iotests/231: Update expected deprecation message

[Connor Kuehl]

On 4/1/21 12:07 PM, Max Reitz wrote:
> On 01.04.21 18:52, Max Reitz wrote:
>> On 01.04.21 17:52, Connor Kuehl wrote:
>>> The deprecation message changed slightly at some point in the past but
>>> the expected output wasn't updated along with it; causing it to fail.
>>> Fix it, so it passes.
>>>
>>> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
>>> ---
>>>   tests/qemu-iotests/231.out | 4 +---
>>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> Uh, well, you know what, I can’t find any version where there was any 
>> other output.  Even back in 66e6a735e97450ac50fcaf40f78600c688534cae, 
>> where this test was introduced, I get this diff.
>>
>> What’s going on there?
> 
> Okay.  So:
> 
> Jeff’s original patch[1] included the “Future versions may cease to 
> parse...” part.  v1 of his subsequent pull request[2] did, too.  But 
> v2[3] didn’t.  Looks like Markus made a comment on v4 of the patch, and 
> then Jeff fixed up the patch in his branch, but didn’t change the test. 
>   In any case it’s clear that the reference output was wrong all along.
> 
> About the “no monitors specified” part...  The only place where I can 
> find “no monitors” is in Jeff’s patches to add this iotest.  I have no 
> idea where that orignated from.
> 
> So:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks! And excellent sleuthing. This accidental conspiracy went much 
farther beyond the git log than I thought...

Connor

> 
> 
> [1]
> https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00282.html
> 
> [2]
> https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00307.html
> 
> [3]
> https://lists.nongnu.org/archive/html/qemu-block/2018-09/msg00592.html
> 

[PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()

[Connor Kuehl]

That's qemu_rbd_unescape()'s job! No need to duplicate the labor.

Furthermore, this was causing some confusion in the parsing logic to
where the caller might test for the presence of a character to split on
like so:

if (strchr(image_name, '/')) {
        found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
	[..]

When qemu_rbd_next_tok() performs unescaping as a side effect, the
parser can get confused thinking that it can split this string, but
really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
"finds" the delimiter and consumes the rest of the token input stream.

This is problematic because qemu_rbd_next_tok() also steals the input
pointer to the token stream and sets it to NULL. This causes a segfault
where the parser expects to split one string into two.

In this case, the parser is determining if a string is a
namespace/image_name pair like so:

	"foo/bar"

And clearly it's looking to split it like so:

	namespace:  foo
	image_name: bar

but if the input is "foo\/bar", it *should* split into

	namespace:  foo\
	image_name: bar

and its subordinate parts can be unescaped after tokenization.

So, instead of tokenizing *and* escaping all at once, do one before the
other to avoid stumbling into a segfault by confusing the parsing logic.

Reported-by: Han Han <hhan@redhat.com>
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
---
 block/rbd.c                | 3 ---
 tests/qemu-iotests/231     | 4 ++++
 tests/qemu-iotests/231.out | 3 +++
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..9bed0863e5 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -123,9 +123,6 @@ static char *qemu_rbd_next_tok(char *src, char delim, char **p)
         if (*end == delim) {
             break;
         }
-        if (*end == '\\' && end[1] != '\0') {
-            end++;
-        }
     }
     if (*end == delim) {
         *p = end + 1;
diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
index 0f66d0ca36..8e6c6447c1 100755
--- a/tests/qemu-iotests/231
+++ b/tests/qemu-iotests/231
@@ -55,6 +55,10 @@ _filter_conf()
 $QEMU_IMG info "json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 2>&1 | _filter_conf
 $QEMU_IMG info "json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}" 2>&1 | _filter_conf
 
+# Regression test: the qemu-img invocation is expected to fail, but it should
+# not seg fault the parser.
+$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
index 747dd221bb..a785a6e859 100644
--- a/tests/qemu-iotests/231.out
+++ b/tests/qemu-iotests/231.out
@@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name: ceph-mon
 qemu-img: Could not open 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': error connecting: No such file or directory
 unable to get monitor info from DNS SRV with service name: ceph-mon
 qemu-img: Could not open 'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}': error connecting: No such file or directory
+Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
+unable to get monitor info from DNS SRV with service name: ceph-mon
+qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or directory
 *** done
-- 
2.30.2

Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()

[Max Reitz]

On 01.04.21 17:52, Connor Kuehl wrote:
> That's qemu_rbd_unescape()'s job! No need to duplicate the labor.
> 
> Furthermore, this was causing some confusion in the parsing logic to
> where the caller might test for the presence of a character to split on
> like so:
> 
> if (strchr(image_name, '/')) {
>          found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
> 	[..]
> 
> When qemu_rbd_next_tok() performs unescaping as a side effect, the
> parser can get confused thinking that it can split this string, but
> really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
> "finds" the delimiter and consumes the rest of the token input stream.

I don’t fully understand.  I understand the strchr() problem and the 
thing you explain next.  But I would have thought that’s a problem of 
using strchr(), i.e. that we’d need a custom function to do strchr() but 
consider escaped characters.  If it’s just about true/false like in your 
example, it could be a new version of qemu_rbd_next_tok() that does not 
modify the string.

Or, well, actually, in your example, one could unconditionally invoke 
qemu_rbd_next_tok() and check whether *found_str is '\0' or not.

> This is problematic because qemu_rbd_next_tok() also steals the input
> pointer to the token stream and sets it to NULL. This causes a segfault
> where the parser expects to split one string into two.

I would say that’s a problem of the caller.  It can pass a different 
variable for the out-pointer, so the input pointer isn’t stolen.

> In this case, the parser is determining if a string is a
> namespace/image_name pair like so:
> 
> 	"foo/bar"
> 
> And clearly it's looking to split it like so:
> 
> 	namespace:  foo
> 	image_name: bar
> 
> but if the input is "foo\/bar", it *should* split into
> 
> 	namespace:  foo\
> 	image_name: bar

Should it?  I would have fully expected it to not be split and the 
parser complains that the input is invalid.  Or, let’s say, 
"foo\/bar/baz” should be split into “foo\/bar” and “baz”.

Max

> and its subordinate parts can be unescaped after tokenization.
> 
> So, instead of tokenizing *and* escaping all at once, do one before the
> other to avoid stumbling into a segfault by confusing the parsing logic.
> 
> Reported-by: Han Han <hhan@redhat.com>
> Fixes: https://bugzilla.redhat.com/1873913
> Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
> ---
>   block/rbd.c                | 3 ---
>   tests/qemu-iotests/231     | 4 ++++
>   tests/qemu-iotests/231.out | 3 +++
>   3 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 9071a00e3f..9bed0863e5 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -123,9 +123,6 @@ static char *qemu_rbd_next_tok(char *src, char delim, char **p)
>           if (*end == delim) {
>               break;
>           }
> -        if (*end == '\\' && end[1] != '\0') {
> -            end++;
> -        }
>       }
>       if (*end == delim) {
>           *p = end + 1;
> diff --git a/tests/qemu-iotests/231 b/tests/qemu-iotests/231
> index 0f66d0ca36..8e6c6447c1 100755
> --- a/tests/qemu-iotests/231
> +++ b/tests/qemu-iotests/231
> @@ -55,6 +55,10 @@ _filter_conf()
>   $QEMU_IMG info "json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=${BOGUS_CONF}'}" 2>&1 | _filter_conf
>   $QEMU_IMG info "json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'${BOGUS_CONF}'}" 2>&1 | _filter_conf
>   
> +# Regression test: the qemu-img invocation is expected to fail, but it should
> +# not seg fault the parser.
> +$QEMU_IMG create "rbd:rbd/aa\/bb:conf=${BOGUS_CONF}" 1M 2>&1 | _filter_conf
> +
>   # success, all done
>   echo "*** done"
>   rm -f $seq.full
> diff --git a/tests/qemu-iotests/231.out b/tests/qemu-iotests/231.out
> index 747dd221bb..a785a6e859 100644
> --- a/tests/qemu-iotests/231.out
> +++ b/tests/qemu-iotests/231.out
> @@ -4,4 +4,7 @@ unable to get monitor info from DNS SRV with service name: ceph-mon
>   qemu-img: Could not open 'json:{'file.driver':'rbd','file.filename':'rbd:rbd/bogus:conf=BOGUS_CONF'}': error connecting: No such file or directory
>   unable to get monitor info from DNS SRV with service name: ceph-mon
>   qemu-img: Could not open 'json:{'file.driver':'rbd','file.pool':'rbd','file.image':'bogus','file.conf':'BOGUS_CONF'}': error connecting: No such file or directory
> +Formatting 'rbd:rbd/aa\/bb:conf=BOGUS_CONF', fmt=raw size=1048576
> +unable to get monitor info from DNS SRV with service name: ceph-mon
> +qemu-img: rbd:rbd/aa\/bb:conf=BOGUS_CONF: error connecting: No such file or directory
>   *** done
> 

Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()

[Connor Kuehl]

On 4/1/21 12:24 PM, Max Reitz wrote:
> On 01.04.21 17:52, Connor Kuehl wrote:
>> That's qemu_rbd_unescape()'s job! No need to duplicate the labor.
>>
>> Furthermore, this was causing some confusion in the parsing logic to
>> where the caller might test for the presence of a character to split on
>> like so:
>>
>> if (strchr(image_name, '/')) {
>>          found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
>>     [..]
>>
>> When qemu_rbd_next_tok() performs unescaping as a side effect, the
>> parser can get confused thinking that it can split this string, but
>> really the delimiter '/' gets unescaped and so qemu_rbd_next_tok() never
>> "finds" the delimiter and consumes the rest of the token input stream.
> 
> I don’t fully understand.  I understand the strchr() problem and the 
> thing you explain next.  But I would have thought that’s a problem of 
> using strchr(), i.e. that we’d need a custom function to do strchr() but 
> consider escaped characters.  If it’s just about true/false like in your 
> example, it could be a new version of qemu_rbd_next_tok() that does not 
> modify the string.

I went back and forth a lot on the different ways this can be fixed 
before sending this, and I agree the most consistent fix here would be 
to add an escape-aware strchr. Initially, adding another libc-like 
function with more side effects wasn't as appealing to me as removing 
the side effects entirely to separate mechanism vs. policy. Your example 
below convinced me that this patch would split the token in unexpected 
ways. I'll send a v2 :-)

Thanks,

Connor

> [..]
> Should it?  I would have fully expected it to not be split and the 
> parser complains that the input is invalid.  Or, let’s say, 
> "foo\/bar/baz” should be split into “foo\/bar” and “baz”.