Pino Toscano
2020-Jan-09 17:40 UTC
Re: [Libguestfs] [PATCH] Fix lossy conversion of Content-Length
On Thursday, 9 January 2020 17:41:58 CET Adrian Ambrożewicz wrote:> W dniu 1/9/2020 o 14:07, Richard W.M. Jones pisze: > > 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 > 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
Adrian Ambrożewicz
2020-Jan-10 09:34 UTC
[Libguestfs] [PATCH v2] Fix lossy conversion of Content-Length
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
Richard W.M. Jones
2020-Jan-10 09:56 UTC
Re: [Libguestfs] [PATCH v2] Fix lossy conversion of Content-Length
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/