[PATCH] Fix lossy conversion of Content-Length
Actual variable holding content length is int64_t, but it was assigned by explicit cast to size_t. On 32-bit systems it's a lossy conversion, so it was replaced by casting to int64_t instead. Signed-off-by: Adrian Ambrożewicz <adrian.ambrozewicz@linux.intel.com> --- plugins/curl/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c index 031bd32..fe1330e 100644 --- a/plugins/curl/curl.c +++ b/plugins/curl/curl.c @@ -389,7 +389,7 @@ curl_open (int readonly) goto err; } - h->exportsize = (size_t) d; + h->exportsize = (int64_t) d; nbdkit_debug ("content length: %" PRIi64, h->exportsize); if (strncasecmp (url, "http://", strlen ("http://")) == 0 || -- 2.17.1
On 1/7/20 4:13 AM, Adrian Ambrożewicz wrote:
Actual variable holding content length is int64_t, but it was assigned by explicit cast to size_t. On 32-bit systems it's a lossy conversion, so it was replaced by casting to int64_t instead.
Signed-off-by: Adrian Ambrożewicz <adrian.ambrozewicz@linux.intel.com> --- plugins/curl/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c index 031bd32..fe1330e 100644 --- a/plugins/curl/curl.c +++ b/plugins/curl/curl.c @@ -389,7 +389,7 @@ curl_open (int readonly) goto err; }
- h->exportsize = (size_t) d; + h->exportsize = (int64_t) d;
Why is a cast needed at all? This is C, an implicit conversion works just as well.
nbdkit_debug ("content length: %" PRIi64, h->exportsize);
if (strncasecmp (url, "http://", strlen ("http://")) == 0 ||
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
W dniu 1/8/2020 o 14:19, Eric Blake pisze:
On 1/7/20 4:13 AM, Adrian Ambrożewicz wrote:
Actual variable holding content length is int64_t, but it was assigned by explicit cast to size_t. On 32-bit systems it's a lossy conversion, so it was replaced by casting to int64_t instead.
Signed-off-by: Adrian Ambrożewicz <adrian.ambrozewicz@linux.intel.com> --- plugins/curl/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c index 031bd32..fe1330e 100644 --- a/plugins/curl/curl.c +++ b/plugins/curl/curl.c @@ -389,7 +389,7 @@ curl_open (int readonly) goto err; }
- h->exportsize = (size_t) d; + h->exportsize = (int64_t) d;
Why is a cast needed at all? This is C, an implicit conversion works just as well. I agree. It was just me being careful with scope of changes :) Should I send new patch? And another question - who is the maintainer and how should I proceed with patch after acceptance on mailing list?
nbdkit_debug ("content length: %" PRIi64, h->exportsize);
if (strncasecmp (url, "http://", strlen ("http://")) == 0 ||
On Wed, Jan 08, 2020 at 07:19:56AM -0600, Eric Blake wrote:
On 1/7/20 4:13 AM, Adrian Ambrożewicz wrote:
Actual variable holding content length is int64_t, but it was assigned by explicit cast to size_t. On 32-bit systems it's a lossy conversion, so it was replaced by casting to int64_t instead.
Signed-off-by: Adrian Ambrożewicz <adrian.ambrozewicz@linux.intel.com> --- plugins/curl/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c index 031bd32..fe1330e 100644 --- a/plugins/curl/curl.c +++ b/plugins/curl/curl.c @@ -389,7 +389,7 @@ curl_open (int readonly) goto err; }
- h->exportsize = (size_t) d; + h->exportsize = (int64_t) d;
Why is a cast needed at all? This is C, an implicit conversion works just as well.
Present since the initial commit, but not present in the qemu block/curl.c driver which is where most of this code was derived from, so I suspect I simply added it "because reasons". But even more to the point, why on earth does the CURLINFO_CONTENT_LENGTH_DOWNLOAD API return a double! Well it seems as if they replaced that API with: https://curl.haxx.se/libcurl/c/CURLINFO_CONTENT_LENGTH_DOWNLOAD_T.html which is what we really ought to use instead. This was added in Curl 7.55.0, released 2 and a half years ago, so I think we ought to use that instead. Adrian could you write a patch to change over to that API? Rich.
nbdkit_debug ("content length: %" PRIi64, h->exportsize);
if (strncasecmp (url, "http://", strlen ("http://")) == 0 ||
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Wed, Jan 08, 2020 at 07:19:56AM -0600, Eric Blake wrote:
On 1/7/20 4:13 AM, Adrian Ambrożewicz wrote:
Actual variable holding content length is int64_t, but it was assigned by explicit cast to size_t. On 32-bit systems it's a lossy conversion, so it was replaced by casting to int64_t instead.
Signed-off-by: Adrian Ambrożewicz <adrian.ambrozewicz@linux.intel.com> --- plugins/curl/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c index 031bd32..fe1330e 100644 --- a/plugins/curl/curl.c +++ b/plugins/curl/curl.c @@ -389,7 +389,7 @@ curl_open (int readonly) goto err; }
- h->exportsize = (size_t) d; + h->exportsize = (int64_t) d;
Why is a cast needed at all? This is C, an implicit conversion works just as well.
Present since the initial commit, but not present in the qemu block/curl.c driver which is where most of this code was derived from, so I suspect I simply added it "because reasons".
But even more to the point, why on earth does the CURLINFO_CONTENT_LENGTH_DOWNLOAD API return a double! Well it seems as if they replaced that API with:
https://curl.haxx.se/libcurl/c/CURLINFO_CONTENT_LENGTH_DOWNLOAD_T.html
which is what we really ought to use instead. This was added in Curl 7.55.0, released 2 and a half years ago, so I think we ought to use that instead. Adrian could you write a patch to change over to that API?
Rich. I see that you've already ACK-ed such change by Pino so I assume my
W dniu 1/9/2020 o 14:07, Richard W.M. Jones pisze: patch is OK? Or should I remove casting altogether?
nbdkit_debug ("content length: %" PRIi64, h->exportsize);
if (strncasecmp (url, "http://", strlen ("http://")) == 0 ||
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
On Thursday, 9 January 2020 17:41:58 CET Adrian Ambrożewicz wrote:
On Wed, Jan 08, 2020 at 07:19:56AM -0600, Eric Blake wrote:
On 1/7/20 4:13 AM, Adrian Ambrożewicz wrote:
Actual variable holding content length is int64_t, but it was assigned by explicit cast to size_t. On 32-bit systems it's a lossy conversion, so it was replaced by casting to int64_t instead.
Signed-off-by: Adrian Ambrożewicz <adrian.ambrozewicz@linux.intel.com> --- plugins/curl/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c index 031bd32..fe1330e 100644 --- a/plugins/curl/curl.c +++ b/plugins/curl/curl.c @@ -389,7 +389,7 @@ curl_open (int readonly) goto err; }
- h->exportsize = (size_t) d; + h->exportsize = (int64_t) d;
Why is a cast needed at all? This is C, an implicit conversion works just as well.
Present since the initial commit, but not present in the qemu block/curl.c driver which is where most of this code was derived from, so I suspect I simply added it "because reasons".
But even more to the point, why on earth does the CURLINFO_CONTENT_LENGTH_DOWNLOAD API return a double! Well it seems as if they replaced that API with:
https://curl.haxx.se/libcurl/c/CURLINFO_CONTENT_LENGTH_DOWNLOAD_T.html
which is what we really ought to use instead. This was added in Curl 7.55.0, released 2 and a half years ago, so I think we ought to use that instead. Adrian could you write a patch to change over to that API?
Rich. I see that you've already ACK-ed such change by Pino so I assume my
W dniu 1/9/2020 o 14:07, Richard W.M. Jones pisze: patch is OK? Or should I remove casting altogether?
My patch used the better API with curl >= 7.55.0. In older versions the issue still exists, and removing the cast should work. Can you please provide a v2 of your patch with that change? Thanks, -- Pino Toscano
From a120342e0d3d20962396e6bf5bd5ac30c66b5983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20Ambro=C5=BCewicz?= <adrian.ambrozewicz@linux.intel.com> Date: Tue, 7 Jan 2020 11:07:08 +0100 Subject: [PATCH 1/1] Fix lossy conversion of Content-Length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Actual variable holding content length is int64_t, but it was assigned by explicit cast to size_t. On 32-bit systems it's a lossy conversion, so it was removed altogether. Signed-off-by: Adrian Ambrożewicz <adrian.ambrozewicz@linux.intel.com> --- plugins/curl/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c index 031bd32..633840a 100644 --- a/plugins/curl/curl.c +++ b/plugins/curl/curl.c @@ -389,7 +389,7 @@ curl_open (int readonly) goto err; } - h->exportsize = (size_t) d; + h->exportsize = d; nbdkit_debug ("content length: %" PRIi64, h->exportsize); if (strncasecmp (url, "http://", strlen ("http://")) == 0 || -- 2.17.1
Thanks - I have pushed this now. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
participants (4)
-
Adrian Ambrożewicz -
Eric Blake -
Pino Toscano -
Richard W.M. Jones