Re: Downloading 3.5 GB ISO

John Boero <[email protected]> wrote Wed, Jun 06, 2007:
> Hi.  I distribute sabayon isos through my shared hosting account.  It's 
> quickest for me to ssh into the host and download from my source 
> directly (thanks to links).  This time I noticed quirky estimates on my 
> larger files.  It looks like there's a signed int for tracking bytes 
> received.

Could you tell us about what version of ELinks you are using?

> Could this be fixed by making it a long?

The off_t type should be used for this kind of thing; it seems Witold
already made such preparations.

> No big deal, just thought I'd make it known.

Thank you for reporting this.

... and now some comments to the patch

> From 6e43bca585238183c6a3f04695b48b755b1b52b6 Mon Sep 17 00:00:00 2001
> From: Witold Filipczyk <[email protected]>
> Date: Thu, 7 Jun 2007 16:16:53 +0200
> Subject: [PATCH] http: used off_t instead of int.
> 
> The progress of files bigger than 2GB is shown properly.
> ---
>  src/protocol/http/http.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/protocol/http/http.c b/src/protocol/http/http.c
> index 62f7c3f..71086c3 100644
> --- a/src/protocol/http/http.c
> +++ b/src/protocol/http/http.c
> @@ -74,13 +74,13 @@ struct http_connection_info {
>  
>  #define LEN_CHUNKED -2 /* == we get data in unknown number of chunks */
>  #define LEN_FINISHED 0
> -	int length;
> +	off_t length;
>  
>          /* Either bytes coming in this chunk yet or "parser state". */
>  #define CHUNK_DATA_END	-3
>  #define CHUNK_ZERO_SIZE	-2
>  #define CHUNK_SIZE	-1
> -	int chunk_remaining;
> +	off_t chunk_remaining;

This change should have some consequences for decompress_data calling
also I think. At least we need to check for overflows before trunkating
and assigning the value of this to the len parameter passed to
decompress_data. The code in question:

	...
        int len;
        int zero = (http->chunk_remaining == CHUNK_ZERO_SIZE);

        if (zero) http->chunk_remaining = 0;
        len = http->chunk_remaining;

        /* Maybe everything necessary didn't come yet.. */
        int_upper_bound(&len, rb->length);
        conn->received += len;
        
        data = decompress_data(conn, rb->data, len, &data_len);
	...

I am not sure if this should also be updated to use strtoll:

        unsigned char *de;
        int n = 0;

        if (l != -1) {
                errno = 0;
                n = strtol(rb->data, (char **) &de, 16);
                if (errno || !*de) {
                        return -1;
                }
        }

        if (l == -1 || de == rb->data) {
                return -1;
        }

        /* Remove everything to the EOLN. */
        kill_buffer_data(rb, l);
        http->chunk_remaining = n;
 
... or whether chunk_remaining should even be off_t, but it clearly
results in fewer changes needed to decompress_data.

The problem with potential overflows should also be investigated with
respect the http_connection_info->length member. The only code where
this could be a problem seems to be this. Since the check already
ensures that http->length is smaller than an already known-good value,
it should be OK, I think:

	read_normal_http_data(struct connection *conn, struct read_buffer *rb)
	{
	        struct http_connection_info *http = conn->info;
	        unsigned char *data;
	        int data_len;
	        int len = rb->length;

        	if (http->length >= 0 && http->length < len) {
        	        /* We won't read more than we have to go. */
        	        len = http->length;
        	}

>          int code;
>  };
> @@ -1007,7 +1007,7 @@ decompress_data(struct connection *conn, unsigned char *data, int len,
>           * early otherwise)). */
>          enum { NORMAL, FINISHING } state = NORMAL;
>          int did_read = 0;
> -	int *length_of_block;
> +	off_t *length_of_block;
>          unsigned char *output = NULL;
>  
>          length_of_block = (http->length == LEN_CHUNKED ? &http->chunk_remaining
> @@ -1761,10 +1761,10 @@ again:
>          d = parse_header(conn->cached->head, "Content-Length", NULL);
>          if (d) {
>                  unsigned char *ep;
> -		int l;
> +		off_t l;
>  
>                  errno = 0;
> -		l = strtol(d, (char **) &ep, 10);
> +		l = strtoll(d, (char **) &ep, 10);
>  
>                  if (!errno && !*ep && l >= 0) {
>                          if (!http->close || POST_HTTP_1_0(version))
> -- 
> 1.5.2.1.851.g432c-dirty

-- 
Jonas Fonseca