Libguestfs - Jan 2020 - Re: [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 at 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