* [PATCH 0/2] Fix segfault in qemu_rbd_parse_filename
@ 2021-04-01 15:52 Connor Kuehl
2021-04-01 15:52 ` [PATCH 1/2] iotests/231: Update expected deprecation message Connor Kuehl
2021-04-01 15:52 ` [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok() Connor Kuehl
0 siblings, 2 replies; 8+ messages in thread
From: Connor Kuehl @ 2021-04-01 15:52 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, dillaman, qemu-devel, mreitz
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] iotests/231: Update expected deprecation message
2021-04-01 15:52 [PATCH 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl
@ 2021-04-01 15:52 ` Connor Kuehl
2021-04-01 16:52 ` Max Reitz
2021-04-01 15:52 ` [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok() Connor Kuehl
1 sibling, 1 reply; 8+ messages in thread
From: Connor Kuehl @ 2021-04-01 15:52 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, dillaman, qemu-devel, mreitz
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()
2021-04-01 15:52 [PATCH 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl
2021-04-01 15:52 ` [PATCH 1/2] iotests/231: Update expected deprecation message Connor Kuehl
@ 2021-04-01 15:52 ` Connor Kuehl
2021-04-01 17:24 ` Max Reitz
1 sibling, 1 reply; 8+ messages in thread
From: Connor Kuehl @ 2021-04-01 15:52 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, dillaman, qemu-devel, mreitz
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iotests/231: Update expected deprecation message
2021-04-01 15:52 ` [PATCH 1/2] iotests/231: Update expected deprecation message Connor Kuehl
@ 2021-04-01 16:52 ` Max Reitz
2021-04-01 17:07 ` Max Reitz
0 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2021-04-01 16:52 UTC (permalink / raw)
To: Connor Kuehl, qemu-block; +Cc: kwolf, dillaman, qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iotests/231: Update expected deprecation message
2021-04-01 16:52 ` Max Reitz
@ 2021-04-01 17:07 ` Max Reitz
2021-04-01 17:10 ` Connor Kuehl
0 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2021-04-01 17:07 UTC (permalink / raw)
To: Connor Kuehl, qemu-block; +Cc: kwolf, dillaman, qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iotests/231: Update expected deprecation message
2021-04-01 17:07 ` Max Reitz
@ 2021-04-01 17:10 ` Connor Kuehl
0 siblings, 0 replies; 8+ messages in thread
From: Connor Kuehl @ 2021-04-01 17:10 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: kwolf, dillaman, qemu-devel
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()
2021-04-01 15:52 ` [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok() Connor Kuehl
@ 2021-04-01 17:24 ` Max Reitz
2021-04-01 18:09 ` Connor Kuehl
0 siblings, 1 reply; 8+ messages in thread
From: Max Reitz @ 2021-04-01 17:24 UTC (permalink / raw)
To: Connor Kuehl, qemu-block; +Cc: kwolf, dillaman, qemu-devel
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok()
2021-04-01 17:24 ` Max Reitz
@ 2021-04-01 18:09 ` Connor Kuehl
0 siblings, 0 replies; 8+ messages in thread
From: Connor Kuehl @ 2021-04-01 18:09 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: kwolf, dillaman, qemu-devel
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”.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-01 18:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01 15:52 [PATCH 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl
2021-04-01 15:52 ` [PATCH 1/2] iotests/231: Update expected deprecation message Connor Kuehl
2021-04-01 16:52 ` Max Reitz
2021-04-01 17:07 ` Max Reitz
2021-04-01 17:10 ` Connor Kuehl
2021-04-01 15:52 ` [PATCH 2/2] block/rbd: Don't unescape in qemu_rbd_next_tok() Connor Kuehl
2021-04-01 17:24 ` Max Reitz
2021-04-01 18:09 ` Connor Kuehl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).