* [PATCH v3 0/2] Fix segfault in qemu_rbd_parse_filename
@ 2021-04-09 14:38 Connor Kuehl
2021-04-09 14:38 ` [PATCH v3 1/2] iotests/231: Update expected deprecation message Connor Kuehl
2021-04-09 14:38 ` [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper Connor Kuehl
0 siblings, 2 replies; 7+ messages in thread
From: Connor Kuehl @ 2021-04-09 14:38 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, dillaman, qemu-devel, mreitz
Connor Kuehl (2):
iotests/231: Update expected deprecation message
block/rbd: Add an escape-aware strchr helper
block/rbd.c | 20 ++++++++++++++++++--
tests/qemu-iotests/231 | 4 ++++
tests/qemu-iotests/231.out | 7 ++++---
3 files changed, 26 insertions(+), 5 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/2] iotests/231: Update expected deprecation message
2021-04-09 14:38 [PATCH v3 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl
@ 2021-04-09 14:38 ` Connor Kuehl
2021-04-21 10:48 ` Stefano Garzarella
2021-04-09 14:38 ` [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper Connor Kuehl
1 sibling, 1 reply; 7+ messages in thread
From: Connor Kuehl @ 2021-04-09 14:38 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, dillaman, qemu-devel, mreitz
The deprecation message in the expected output has technically been
wrong since the wrong version of a patch was applied to it. Because of
this, the test fails. Correct the expected output so that it passes.
Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
Reviewed-by: Max Reitz <mreitz@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] 7+ messages in thread
* [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper
2021-04-09 14:38 [PATCH v3 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl
2021-04-09 14:38 ` [PATCH v3 1/2] iotests/231: Update expected deprecation message Connor Kuehl
@ 2021-04-09 14:38 ` Connor Kuehl
2021-04-21 11:04 ` Stefano Garzarella
1 sibling, 1 reply; 7+ messages in thread
From: Connor Kuehl @ 2021-04-09 14:38 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, dillaman, qemu-devel, mreitz
Sometimes the parser needs to further split a token it has collected
from the token input stream. Right now, it does a cursory check to see
if the relevant characters appear in the token to determine if it should
break it down further.
However, qemu_rbd_next_tok() will escape characters as it removes tokens
from the token stream and plain strchr() won't. This can make the
initial strchr() check slightly misleading since it implies
qemu_rbd_next_tok() will find the token and split on it, except the
reality is that qemu_rbd_next_tok() will pass over it if it is escaped.
Use a custom strchr to avoid mixing escaped and unescaped string
operations.
Reported-by: Han Han <hhan@redhat.com>
Fixes: https://bugzilla.redhat.com/1873913
Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
---
v2 -> v3:
* Update qemu_rbd_strchr to only skip if there's a delimiter AND the
next character is not the NUL terminator
block/rbd.c | 20 ++++++++++++++++++--
tests/qemu-iotests/231 | 4 ++++
tests/qemu-iotests/231.out | 3 +++
3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..291e3f09e1 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char delim, char **p)
return src;
}
+static char *qemu_rbd_strchr(char *src, char delim)
+{
+ char *p;
+
+ for (p = src; *p; ++p) {
+ if (*p == delim) {
+ return p;
+ }
+ if (*p == '\\' && p[1] != '\0') {
+ ++p;
+ }
+ }
+
+ return NULL;
+}
+
static void qemu_rbd_unescape(char *src)
{
char *p;
@@ -171,7 +187,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
qemu_rbd_unescape(found_str);
qdict_put_str(options, "pool", found_str);
- if (strchr(p, '@')) {
+ if (qemu_rbd_strchr(p, '@')) {
image_name = qemu_rbd_next_tok(p, '@', &p);
found_str = qemu_rbd_next_tok(p, ':', &p);
@@ -181,7 +197,7 @@ static void qemu_rbd_parse_filename(const char *filename, QDict *options,
image_name = qemu_rbd_next_tok(p, ':', &p);
}
/* Check for namespace in the image_name */
- if (strchr(image_name, '/')) {
+ if (qemu_rbd_strchr(image_name, '/')) {
found_str = qemu_rbd_next_tok(image_name, '/', &image_name);
qemu_rbd_unescape(found_str);
qdict_put_str(options, "namespace", found_str);
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] 7+ messages in thread
* Re: [PATCH v3 1/2] iotests/231: Update expected deprecation message
2021-04-09 14:38 ` [PATCH v3 1/2] iotests/231: Update expected deprecation message Connor Kuehl
@ 2021-04-21 10:48 ` Stefano Garzarella
0 siblings, 0 replies; 7+ messages in thread
From: Stefano Garzarella @ 2021-04-21 10:48 UTC (permalink / raw)
To: Connor Kuehl; +Cc: kwolf, qemu-devel, qemu-block, mreitz
On Fri, Apr 09, 2021 at 09:38:53AM -0500, Connor Kuehl wrote:
>The deprecation message in the expected output has technically been
>wrong since the wrong version of a patch was applied to it. Because of
>this, the test fails. Correct the expected output so that it passes.
>
>Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
>Reviewed-by: Max Reitz <mreitz@redhat.com>
>---
> tests/qemu-iotests/231.out | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>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 [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper
2021-04-09 14:38 ` [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper Connor Kuehl
@ 2021-04-21 11:04 ` Stefano Garzarella
2021-04-21 21:15 ` Connor Kuehl
0 siblings, 1 reply; 7+ messages in thread
From: Stefano Garzarella @ 2021-04-21 11:04 UTC (permalink / raw)
To: Connor Kuehl; +Cc: kwolf, qemu-devel, qemu-block, mreitz
On Fri, Apr 09, 2021 at 09:38:54AM -0500, Connor Kuehl wrote:
>Sometimes the parser needs to further split a token it has collected
>from the token input stream. Right now, it does a cursory check to see
>if the relevant characters appear in the token to determine if it should
>break it down further.
>
>However, qemu_rbd_next_tok() will escape characters as it removes tokens
>from the token stream and plain strchr() won't. This can make the
>initial strchr() check slightly misleading since it implies
>qemu_rbd_next_tok() will find the token and split on it, except the
>reality is that qemu_rbd_next_tok() will pass over it if it is escaped.
>
>Use a custom strchr to avoid mixing escaped and unescaped string
>operations.
>
>Reported-by: Han Han <hhan@redhat.com>
>Fixes: https://bugzilla.redhat.com/1873913
>Signed-off-by: Connor Kuehl <ckuehl@redhat.com>
>---
> v2 -> v3:
> * Update qemu_rbd_strchr to only skip if there's a delimiter AND the
> next character is not the NUL terminator
>
> block/rbd.c | 20 ++++++++++++++++++--
> tests/qemu-iotests/231 | 4 ++++
> tests/qemu-iotests/231.out | 3 +++
> 3 files changed, 25 insertions(+), 2 deletions(-)
>
>diff --git a/block/rbd.c b/block/rbd.c
>index 9071a00e3f..291e3f09e1 100644
>--- a/block/rbd.c
>+++ b/block/rbd.c
>@@ -134,6 +134,22 @@ static char *qemu_rbd_next_tok(char *src, char delim, char **p)
> return src;
> }
>
>+static char *qemu_rbd_strchr(char *src, char delim)
>+{
>+ char *p;
>+
>+ for (p = src; *p; ++p) {
>+ if (*p == delim) {
>+ return p;
>+ }
>+ if (*p == '\\' && p[1] != '\0') {
>+ ++p;
>+ }
>+ }
>+
>+ return NULL;
>+}
>+
IIUC this is similar to the code used in qemu_rbd_next_tok().
To avoid code duplication can we use this new function inside it?
I mean something like this (not tested):
diff --git a/block/rbd.c b/block/rbd.c
index f098a89c7b..eb6a839362 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -119,15 +119,8 @@ static char *qemu_rbd_next_tok(char *src, char delim, char **p)
*p = NULL;
- for (end = src; *end; ++end) {
- if (*end == delim) {
- break;
- }
- if (*end == '\\' && end[1] != '\0') {
- end++;
- }
- }
- if (*end == delim) {
+ end = qemu_rbd_strchr(src, delim);
+ if (end && *end == delim) {
*p = end + 1;
*end = '\0';
}
The rest LGTM!
Thanks for fixing this issue,
Stefano
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper
2021-04-21 11:04 ` Stefano Garzarella
@ 2021-04-21 21:15 ` Connor Kuehl
2021-04-22 7:11 ` Stefano Garzarella
0 siblings, 1 reply; 7+ messages in thread
From: Connor Kuehl @ 2021-04-21 21:15 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: kwolf, qemu-devel, qemu-block, mreitz
On 4/21/21 6:04 AM, Stefano Garzarella wrote:
>> +static char *qemu_rbd_strchr(char *src, char delim)
>> +{
>> + char *p;
>> +
>> + for (p = src; *p; ++p) {
>> + if (*p == delim) {
>> + return p;
>> + }
>> + if (*p == '\\' && p[1] != '\0') {
>> + ++p;
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>
> IIUC this is similar to the code used in qemu_rbd_next_tok().
> To avoid code duplication can we use this new function inside it?
Hi Stefano! Good catch. I think you're right. I've incorporated your
suggestion into my next revision. The only thing I changed was the
if-statement from:
if (end && *end == delim) {
to:
if (end) {
Since qemu_rbd_strchr() will only ever return a non-NULL pointer if it
was able to find 'delim'.
Connor
>
> I mean something like this (not tested):
>
> diff --git a/block/rbd.c b/block/rbd.c
> index f098a89c7b..eb6a839362 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -119,15 +119,8 @@ static char *qemu_rbd_next_tok(char *src, char delim, char **p)
>
> *p = NULL;
>
> - for (end = src; *end; ++end) {
> - if (*end == delim) {
> - break;
> - }
> - if (*end == '\\' && end[1] != '\0') {
> - end++;
> - }
> - }
> - if (*end == delim) {
> + end = qemu_rbd_strchr(src, delim);
> + if (end && *end == delim) {
> *p = end + 1;
> *end = '\0';
> }
>
>
> The rest LGTM!
>
> Thanks for fixing this issue,
> Stefano
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper
2021-04-21 21:15 ` Connor Kuehl
@ 2021-04-22 7:11 ` Stefano Garzarella
0 siblings, 0 replies; 7+ messages in thread
From: Stefano Garzarella @ 2021-04-22 7:11 UTC (permalink / raw)
To: Connor Kuehl; +Cc: kwolf, qemu-devel, qemu-block, mreitz
Hi Connor,
On Wed, Apr 21, 2021 at 04:15:42PM -0500, Connor Kuehl wrote:
>On 4/21/21 6:04 AM, Stefano Garzarella wrote:
>>> +static char *qemu_rbd_strchr(char *src, char delim)
>>> +{
>>> + char *p;
>>> +
>>> + for (p = src; *p; ++p) {
>>> + if (*p == delim) {
>>> + return p;
>>> + }
>>> + if (*p == '\\' && p[1] != '\0') {
>>> + ++p;
>>> + }
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>
>> IIUC this is similar to the code used in qemu_rbd_next_tok().
>> To avoid code duplication can we use this new function inside it?
>
>Hi Stefano! Good catch. I think you're right. I've incorporated your
>suggestion into my next revision. The only thing I changed was the
>if-statement from:
>
> if (end && *end == delim) {
>
>to:
>
> if (end) {
>
>Since qemu_rbd_strchr() will only ever return a non-NULL pointer if it
>was able to find 'delim'.
Yeah, definitely better :-)
Thanks,
Stefano
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-22 7:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-09 14:38 [PATCH v3 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl
2021-04-09 14:38 ` [PATCH v3 1/2] iotests/231: Update expected deprecation message Connor Kuehl
2021-04-21 10:48 ` Stefano Garzarella
2021-04-09 14:38 ` [PATCH v3 2/2] block/rbd: Add an escape-aware strchr helper Connor Kuehl
2021-04-21 11:04 ` Stefano Garzarella
2021-04-21 21:15 ` Connor Kuehl
2021-04-22 7:11 ` Stefano Garzarella
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).