| Message ID | 20240627153059.589070-1-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block/curl: explicitly assert that strchr returns non-NULL value | expand |
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 >
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 >> >
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)) {
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(-)