[Qemu-devel] [PATCH for-2.9?] block/nfs: Do not strcmp() with NULL

[Qemu-devel] [PATCH for-2.9?] block/nfs: Do not strcmp() with NULL

[Max Reitz]

Parsing the URI is not required to give us a scheme; uri->scheme may be
NULL.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nfs.c b/block/nfs.c
index 0816678307..0c7d5619fe 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -83,7 +83,7 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
         error_setg(errp, "Invalid URI specified");
         goto out;
     }
-    if (strcmp(uri->scheme, "nfs") != 0) {
+    if (!uri->scheme || strcmp(uri->scheme, "nfs") != 0) {
         error_setg(errp, "URI scheme must be 'nfs'");
         goto out;
     }
-- 
2.12.2

Re: [Qemu-devel] [PATCH for-2.9?] block/nfs: Do not strcmp() with NULL

[Kevin Wolf]

Am 11.04.2017 um 15:13 hat Max Reitz geschrieben:
> Parsing the URI is not required to give us a scheme; uri->scheme may be
> NULL.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH for-2.9?] block/nfs: Do not strcmp() with NULL

[Peter Maydell]

On 11 April 2017 at 14:13, Max Reitz <mreitz@redhat.com> wrote:
> Parsing the URI is not required to give us a scheme; uri->scheme may be
> NULL.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/nfs.c b/block/nfs.c
> index 0816678307..0c7d5619fe 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -83,7 +83,7 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
>          error_setg(errp, "Invalid URI specified");
>          goto out;
>      }
> -    if (strcmp(uri->scheme, "nfs") != 0) {
> +    if (!uri->scheme || strcmp(uri->scheme, "nfs") != 0) {
>          error_setg(errp, "URI scheme must be 'nfs'");
>          goto out;
>      }

Just as a record of IRC discussion, a lot of the callers of uri_parse()
get this wrong; for instance
 qemu-img info -f nbd blub#://
 qemu-img info -f sheepdog blub#://
both segfault. So since this isn't new (2.8 behaved the same way) I think
we should not try to fix this at this point in the 2.9 release cycle.
For 2.10 we might consider whether there's a change we could make to
the uri_parse() API that makes this mistake less easy. (For instance
do any of the users really ever want to handle a relative URL without
a schema?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.9?] block/nfs: Do not strcmp() with NULL

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2637 bytes --]

On 11.04.2017 15:30, Peter Maydell wrote:
> On 11 April 2017 at 14:13, Max Reitz <mreitz@redhat.com> wrote:
>> Parsing the URI is not required to give us a scheme; uri->scheme may be
>> NULL.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/nfs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/nfs.c b/block/nfs.c
>> index 0816678307..0c7d5619fe 100644
>> --- a/block/nfs.c
>> +++ b/block/nfs.c
>> @@ -83,7 +83,7 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
>>          error_setg(errp, "Invalid URI specified");
>>          goto out;
>>      }
>> -    if (strcmp(uri->scheme, "nfs") != 0) {
>> +    if (!uri->scheme || strcmp(uri->scheme, "nfs") != 0) {
>>          error_setg(errp, "URI scheme must be 'nfs'");
>>          goto out;
>>      }
> 
> Just as a record of IRC discussion, a lot of the callers of uri_parse()
> get this wrong; for instance
>  qemu-img info -f nbd blub#://
>  qemu-img info -f sheepdog blub#://
> both segfault.

Yes, thanks for catching this.

>                So since this isn't new (2.8 behaved the same way) I think
> we should not try to fix this at this point in the 2.9 release cycle.

And also because it'll always be a NULL pointer dereference, so it's not
*too* bad.

(If it had been just the single place in block/nfs.c, I think it would
have made sense to still take this into 2.9 (because the fix is trivial
and obviously makes sense), but sprinkling this "all over the place"
(well, 8 lines changed) is a bit different.)

> For 2.10 we might consider whether there's a change we could make to
> the uri_parse() API that makes this mistake less easy. (For instance
> do any of the users really ever want to handle a relative URL without
> a schema?)

Well, gluster (the only caller of uri_parse() which handles NULL scheme
correctly) just defaults to a scheme then. So changing this might mean
to break compatibility.

In my opinion, if someone wants to improve uri_parse(), great, but I'm
neither the maintainer of util/uri.c (not that we really have one...),
nor have I ever touched it before, so I'm not exactly avid to be the one
to do so.

I think the number of callers is manageable (5), they're all in the
block layer, they all do roughly the same with the result, and they all
do the appropriate NULL checks except for URI.scheme. So just
(trivially) fixing those 8 lines seems to be much easier and good enough
from my perspective.

(Rather than giving uri_parse() a new interface which might introduce
again new bugs...)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH for-2.9?] block/nfs: Do not strcmp() with NULL

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Parsing the URI is not required to give us a scheme; uri->scheme may be
> NULL.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/nfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/nfs.c b/block/nfs.c
> index 0816678307..0c7d5619fe 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -83,7 +83,7 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
>          error_setg(errp, "Invalid URI specified");
>          goto out;
>      }
> -    if (strcmp(uri->scheme, "nfs") != 0) {
> +    if (!uri->scheme || strcmp(uri->scheme, "nfs") != 0) {
>          error_setg(errp, "URI scheme must be 'nfs'");
>          goto out;
>      }

Consider g_strcmp0().

Re: [Qemu-devel] [PATCH for-2.9?] block/nfs: Do not strcmp() with NULL

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

On 11.04.2017 15:58, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> Parsing the URI is not required to give us a scheme; uri->scheme may be
>> NULL.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/nfs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/nfs.c b/block/nfs.c
>> index 0816678307..0c7d5619fe 100644
>> --- a/block/nfs.c
>> +++ b/block/nfs.c
>> @@ -83,7 +83,7 @@ static int nfs_parse_uri(const char *filename, QDict *options, Error **errp)
>>          error_setg(errp, "Invalid URI specified");
>>          goto out;
>>      }
>> -    if (strcmp(uri->scheme, "nfs") != 0) {
>> +    if (!uri->scheme || strcmp(uri->scheme, "nfs") != 0) {
>>          error_setg(errp, "URI scheme must be 'nfs'");
>>          goto out;
>>      }
> 
> Consider g_strcmp0().

Nice, thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]