[PATCH] block/curl: explicitly assert that strchr returns non-NULL value

[PATCH] block/curl: explicitly assert that strchr returns non-NULL value

[Vladimir Sementsov-Ogievskiy]

strchr may return NULL if colon is not found. It seems clearer to
assert explicitly that we don't expect it here, than dereference 1 in
the next line.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/curl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 419f7c89ef..ccfffd6c12 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -219,7 +219,9 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
         && g_ascii_strncasecmp(header, accept_ranges,
                                strlen(accept_ranges)) == 0) {
 
-        char *p = strchr(header, ':') + 1;
+        char *p = strchr(header, ':');
+        assert(p != NULL);
+        p += 1;
 
         /* Skip whitespace between the header name and value. */
         while (p < end && *p && g_ascii_isspace(*p)) {
-- 
2.34.1

Re: [PATCH] block/curl: explicitly assert that strchr returns non-NULL value

[Kevin Wolf]

Am 27.06.2024 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> strchr may return NULL if colon is not found. It seems clearer to
> assert explicitly that we don't expect it here, than dereference 1 in
> the next line.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  block/curl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 419f7c89ef..ccfffd6c12 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -219,7 +219,9 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>          && g_ascii_strncasecmp(header, accept_ranges,
>                                 strlen(accept_ranges)) == 0) {
>  
> -        char *p = strchr(header, ':') + 1;
> +        char *p = strchr(header, ':');
> +        assert(p != NULL);
> +        p += 1;

I'm not sure if this is actually much clearer because it doesn't say why
we don't expect NULL here. If you don't look at the context, it almost
looks like an assert() where proper error handling is needed. If you do,
then the original line is clear enough.

My first thought was that maybe what we want is a comment, but we
actually already know where the colon is. So how about this instead:

    char *p = header + strlen(accept_ranges);

Kevin

>          /* Skip whitespace between the header name and value. */
>          while (p < end && *p && g_ascii_isspace(*p)) {
> -- 
> 2.34.1
> 

Re: [PATCH] block/curl: explicitly assert that strchr returns non-NULL value

[Vladimir Sementsov-Ogievskiy]

On 27.06.24 21:05, Kevin Wolf wrote:
> Am 27.06.2024 um 17:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> strchr may return NULL if colon is not found. It seems clearer to
>> assert explicitly that we don't expect it here, than dereference 1 in
>> the next line.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   block/curl.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 419f7c89ef..ccfffd6c12 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -219,7 +219,9 @@ static size_t curl_header_cb(void *ptr, size_t size, size_t nmemb, void *opaque)
>>           && g_ascii_strncasecmp(header, accept_ranges,
>>                                  strlen(accept_ranges)) == 0) {
>>   
>> -        char *p = strchr(header, ':') + 1;
>> +        char *p = strchr(header, ':');
>> +        assert(p != NULL);
>> +        p += 1;
> 
> I'm not sure if this is actually much clearer because it doesn't say why
> we don't expect NULL here. If you don't look at the context, it almost
> looks like an assert() where proper error handling is needed. If you do,
> then the original line is clear enough.
> 
> My first thought was that maybe what we want is a comment, but we
> actually already know where the colon is. So how about this instead:
> 
>      char *p = header + strlen(accept_ranges);
> 

Oh, right. That's better.

> 
>>           /* Skip whitespace between the header name and value. */
>>           while (p < end && *p && g_ascii_isspace(*p)) {
>> -- 
>> 2.34.1
>>
> 

-- 
Best regards,
Vladimir