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

[Max Reitz <mreitz@redhat.com>]

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
>