| Message ID | 20210409143854.138177-3-ckuehl@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix segfault in qemu_rbd_parse_filename | expand |
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
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 >
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
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
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(-)