| Message ID | 20170411131327.10734-1-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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
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
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().
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
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; }
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(-)