[v3,2/2] block/rbd: Add an escape-aware strchr helper

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(-)

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