* [PATCH v2 0/2] Fix segfault in qemu_rbd_parse_filename @ 2021-04-01 21:01 Connor Kuehl 2021-04-01 21:01 ` [PATCH v2 1/2] iotests/231: Update expected deprecation message Connor Kuehl 2021-04-01 21:01 ` [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper Connor Kuehl 0 siblings, 2 replies; 6+ messages in thread From: Connor Kuehl @ 2021-04-01 21:01 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, dillaman, qemu-devel, mreitz Replaced the change to qemu_rbd_next_tok with a standalone escape-aware helper for patch 2. 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] 6+ messages in thread
* [PATCH v2 1/2] iotests/231: Update expected deprecation message 2021-04-01 21:01 [PATCH v2 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl @ 2021-04-01 21:01 ` Connor Kuehl 2021-04-01 21:01 ` [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper Connor Kuehl 1 sibling, 0 replies; 6+ messages in thread From: Connor Kuehl @ 2021-04-01 21:01 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> --- Reworded the commit log and added Max's R-b. 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] 6+ messages in thread
* [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper 2021-04-01 21:01 [PATCH v2 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl 2021-04-01 21:01 ` [PATCH v2 1/2] iotests/231: Update expected deprecation message Connor Kuehl @ 2021-04-01 21:01 ` Connor Kuehl 2021-04-06 14:24 ` Max Reitz 1 sibling, 1 reply; 6+ messages in thread From: Connor Kuehl @ 2021-04-01 21:01 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> --- 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..c0e4d4a952 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; + } + } + + 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] 6+ messages in thread
* Re: [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper 2021-04-01 21:01 ` [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper Connor Kuehl @ 2021-04-06 14:24 ` Max Reitz 2021-04-09 14:05 ` Connor Kuehl 0 siblings, 1 reply; 6+ messages in thread From: Max Reitz @ 2021-04-06 14:24 UTC (permalink / raw) To: Connor Kuehl, qemu-block; +Cc: kwolf, dillaman, qemu-devel On 01.04.21 23:01, 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> > --- > 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..c0e4d4a952 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; > + } > + } > + > + return NULL; > +} > + So I thought you could make qemu_rbd_do_next_tok() to do this. (I didn’t say you should, but bear with me.) That would be possible by giving it a new parameter (e.g. @find), and if that is set, return @end if *end == delim after the loop, and NULL otherwise. Now, if you add wrapper functions to make it nice, there’s not much more difference in lines added compared to just adding a new function, but it does mean your function should basically be the same as qemu_rbd_next_tok(), except that no splitting happens, that there is no *p, and that @end is returned instead of @src. So there is one difference, and that is that qemu_rbd_next_tok() has this condition to skip escaped characters: if (*end == '\\' && end[1] != '\0') { where qemu_rbd_strchr() has only: if (*p == '\\') { And I think qemu_rbd_next_tok() is right; if the string in question has a trailing backslash, qemu_rbd_strchr() will ignore the final NUL and continue searching past the end of the string. Max ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper 2021-04-06 14:24 ` Max Reitz @ 2021-04-09 14:05 ` Connor Kuehl 2021-04-09 14:19 ` Max Reitz 0 siblings, 1 reply; 6+ messages in thread From: Connor Kuehl @ 2021-04-09 14:05 UTC (permalink / raw) To: Max Reitz, qemu-block; +Cc: kwolf, dillaman, qemu-devel On 4/6/21 9:24 AM, Max Reitz wrote: > On 01.04.21 23:01, Connor Kuehl wrote: >> [..] >> diff --git a/block/rbd.c b/block/rbd.c >> index 9071a00e3f..c0e4d4a952 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; >> + } >> + } >> + >> + return NULL; >> +} >> + > > So I thought you could make qemu_rbd_do_next_tok() to do this. (I > didn’t say you should, but bear with me.) That would be possible by > giving it a new parameter (e.g. @find), and if that is set, return @end > if *end == delim after the loop, and NULL otherwise. > > Now, if you add wrapper functions to make it nice, there’s not much more > difference in lines added compared to just adding a new function, but it > does mean your function should basically be the same as > qemu_rbd_next_tok(), except that no splitting happens, that there is no > *p, and that @end is returned instead of @src. Do you have a strong preference for this? I agree that qemu_rbd_next_tok() could grow this functionality, but I think it'd be simpler to keep it separate in the form of qemu_rbd_strchr(). > > So there is one difference, and that is that qemu_rbd_next_tok() has > this condition to skip escaped characters: > > if (*end == '\\' && end[1] != '\0') { > > where qemu_rbd_strchr() has only: > > if (*p == '\\') { > > And I think qemu_rbd_next_tok() is right; if the string in question has > a trailing backslash, qemu_rbd_strchr() will ignore the final NUL and > continue searching past the end of the string. Aha, good catch. I'll fix this up. Thank you, Connor ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper 2021-04-09 14:05 ` Connor Kuehl @ 2021-04-09 14:19 ` Max Reitz 0 siblings, 0 replies; 6+ messages in thread From: Max Reitz @ 2021-04-09 14:19 UTC (permalink / raw) To: Connor Kuehl, qemu-block; +Cc: kwolf, dillaman, qemu-devel On 09.04.21 16:05, Connor Kuehl wrote: > On 4/6/21 9:24 AM, Max Reitz wrote: >> On 01.04.21 23:01, Connor Kuehl wrote: >>> [..] >>> diff --git a/block/rbd.c b/block/rbd.c >>> index 9071a00e3f..c0e4d4a952 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; >>> + } >>> + } >>> + >>> + return NULL; >>> +} >>> + >> >> So I thought you could make qemu_rbd_do_next_tok() to do this. (I >> didn’t say you should, but bear with me.) That would be possible by >> giving it a new parameter (e.g. @find), and if that is set, return >> @end if *end == delim after the loop, and NULL otherwise. >> >> Now, if you add wrapper functions to make it nice, there’s not much >> more difference in lines added compared to just adding a new function, >> but it does mean your function should basically be the same as >> qemu_rbd_next_tok(), except that no splitting happens, that there is >> no *p, and that @end is returned instead of @src. > > Do you have a strong preference for this? I agree that > qemu_rbd_next_tok() could grow this functionality, but I think it'd be > simpler to keep it separate in the form of qemu_rbd_strchr(). Oh, no, no. I mostly said this so it would be clear why both functions should basically have the same structure, i.e. why a difference in structure might be a sign that something’s wrong. Sorry if I came across as too verbose. >> So there is one difference, and that is that qemu_rbd_next_tok() has >> this condition to skip escaped characters: >> >> if (*end == '\\' && end[1] != '\0') { >> >> where qemu_rbd_strchr() has only: >> >> if (*p == '\\') { >> >> And I think qemu_rbd_next_tok() is right; if the string in question >> has a trailing backslash, qemu_rbd_strchr() will ignore the final NUL >> and continue searching past the end of the string. > > Aha, good catch. I'll fix this up. Thanks! Max ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-09 14:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-01 21:01 [PATCH v2 0/2] Fix segfault in qemu_rbd_parse_filename Connor Kuehl 2021-04-01 21:01 ` [PATCH v2 1/2] iotests/231: Update expected deprecation message Connor Kuehl 2021-04-01 21:01 ` [PATCH v2 2/2] block/rbd: Add an escape-aware strchr helper Connor Kuehl 2021-04-06 14:24 ` Max Reitz 2021-04-09 14:05 ` Connor Kuehl 2021-04-09 14:19 ` Max Reitz
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).