Re: [Bug-wget] Unit test case for parse_content_range()

[Darshit Shah]

Hi Ander,

Thanks for testing!
I generally have the -std=gnu99 CFLAG enabled so I didn't realize the
error. I've pushed a fix to the repository.

On Mon, Aug 31, 2015 at 3:59 AM, Ander Juaristi  wrote:
> On 08/30/2015 05:57 PM, Darshit Shah wrote:
>>
>> I've attached an updated patch with some test cases and a couple of
>> fixes to the parsing logic.
>>
>
> Hi,
>
> The latest pull fails with the following error:
>
> http.c: In function 'test_parse_range_header':
> http.c:4933:3: error: 'for' loop initial declarations are only allowed in
> C99 mode
>for (unsigned i = 0; i < countof (test_array); i++)
>^
> http.c:4933:3: note: use option -std=c99 or -std=gnu99 to compile your code
>
> I ran './configure' without arguments (ie. default flags):
>
>   Compiler:  gcc
>   CFlags:-I/usr/include/p11-kit-1   -DHAVE_LIBGNUTLS   -DNDEBUG
>   LDFlags:
>   Libs:  -lgnutls   -lz
>   SSL:   gnutls
>
> Regards,
> - AJ
>



-- 
Thanking You,
Darshit Shah

Re: [Bug-wget] Unit test case for parse_content_range()

[Darshit Shah]

I've attached an updated patch with some test cases and a couple of
fixes to the parsing logic.

On Sun, Aug 30, 2015 at 11:04 AM, Darshit Shah dar...@gmail.com wrote:
 On Sun, Aug 30, 2015 at 12:51 AM, Tim Rühsen tim.rueh...@gmx.de wrote:
 Am Samstag, 29. August 2015, 23:13:23 schrieb Darshit Shah:
 I've written a unit test for the parse_content_range() method.
 However, I haven't yet populated it with various test cases.
 Sharing the patch for the unit test here. I will add more test cases
 for this test later.

 Kindly do review the patch. If no one complains, I'll push it in a
 couple of days.

 Hi Darshit,

 some of the 'valid' tests

 On closer inspection, some of these are *NOT* valid Header values. But
 Wget currently passes them. This is a parsing bug in my opinion.

 RFC 7233 states:

  A Content-Range field value is invalid if it contains a
byte-range-resp that has a last-byte-pos value less than its
first-byte-pos value, or a complete-length value less than or equal
to its last-byte-pos value.  The recipient of an invalid
Content-Range MUST NOT attempt to recombine the received content with
a stored representation.

 Based on this, the first two examples provided are illegal. Similarly
 a header value such as:
 { bytes 100-99/1000, 100, 99, 1000}
 should also be very clearly illegal, but Wget currently allows it. I'm
 not sure about the behaviour of the program on receipt of such a
 header, but the function should clearly be failing on this test.

 0-max
 { bytes 0-1000/1000, 0, 1000, 1000}
 non0-max
 { bytes 1-1000/1000, 1, 1000, 1000}
 0-valid
 { bytes 0-500/1000, 0, 500, 1000}
 non0-valid
 { bytes 1-500/1000, 1, 500, 1000}
 0-(max-1)
 { bytes 0-999/1000, 0, 999, 1000}
 non0-(max-1)
 { bytes 1-999/1000, 1, 999, 1000}

 And please add some tests using =2^31 and =2^32 as values.

 Regards, Tim



 --
 Thanking You,
 Darshit Shah



-- 
Thanking You,
Darshit Shah
From 727ca78eb615b04f159d5c76cc1bae20119b628d Mon Sep 17 00:00:00 2001
From: Darshit Shah dar...@gmail.com
Date: Sat, 29 Aug 2015 23:08:39 +0530
Subject: [PATCH] Add unit test for parse_content_range() method

* http.c (test_parse_range_header): New function to test the
function for parsing the HTTP/1.1 Content-Range header.
* test.[ch]: Same
* http.c (parse_content_range): Fix parsing code. Fail on scenarios
mentioned in rfc 7233.
---
 src/http.c | 53 -
 src/test.c |  1 +
 src/test.h |  1 +
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/http.c b/src/http.c
index e96cad7..20275ea 100644
--- a/src/http.c
+++ b/src/http.c
@@ -909,9 +909,13 @@ parse_content_range (const char *hdr, wgint *first_byte_ptr,
   ++hdr;
   for (num = 0; c_isdigit (*hdr); hdr++)
 num = 10 * num + (*hdr - '0');
-  if (*hdr != '/' || !c_isdigit (*(hdr + 1)))
+  if (*hdr != '/')
 return false;
   *last_byte_ptr = num;
+  if (!(c_isdigit (*(hdr + 1)) || *(hdr + 1) == '*'))
+return false;
+  if (*last_byte_ptr  *first_byte_ptr)
+return false;
   ++hdr;
   if (*hdr == '*')
 num = -1;
@@ -919,6 +923,8 @@ parse_content_range (const char *hdr, wgint *first_byte_ptr,
 for (num = 0; c_isdigit (*hdr); hdr++)
   num = 10 * num + (*hdr - '0');
   *entity_length_ptr = num;
+  if ((*entity_length_ptr = *last_byte_ptr)  *entity_length_ptr != -1)
+return false;
   return true;
 }
 
@@ -4892,6 +4898,51 @@ ensure_extension (struct http_stat *hs, const char *ext, int *dt)
 }
 
 #ifdef TESTING
+
+const char *
+test_parse_range_header(void)
+{
+  static const struct {
+const char * rangehdr;
+const wgint firstbyte;
+const wgint lastbyte;
+const wgint length;
+const bool shouldPass;
+  } test_array[] = {
+  { bytes 0-1000/1000, 0, 1000, 1000, false },
+  { bytes 0-999/1000, 0, 999, 1000, true },
+  { bytes 100-99/1000, 100, 99, 1000, false },
+  { bytes 100-100/1000, 100, 100, 1000, true },
+  { bytes 0-1000/1, 0, 1000, 1, true },
+  { bytes 1-999/1000, 1, 999, 1000, true },
+  { bytes 42-1233/1234, 42, 1233, 1234, true },
+  { bytes 42-1233/*, 42, 1233, -1, true },
+  { bytes 0-2147483648/2147483649, 0, 2147483648, 2147483649, true },
+  { bytes 2147483648-4294967296/4294967297, 2147483648, 4294967296, 4294967297, true }
+  };
+
+  wgint firstbyteptr[sizeof(wgint)];
+  wgint lastbyteptr[sizeof(wgint)];
+  wgint lengthptr[sizeof(wgint)];
+  bool result;
+  for (unsigned i = 0; i  countof (test_array); i++)
+{
+  result = parse_content_range (test_array[i].rangehdr, firstbyteptr, lastbyteptr, lengthptr);
+#if 0
+  printf (%ld %ld\n, test_array[i].firstbyte, *firstbyteptr);
+  printf (%ld %ld\n, test_array[i].lastbyte, *lastbyteptr);
+  printf (%ld %ld\n, test_array[i].length, *lengthptr);
+  printf (\n);
+#endif
+  mu_assert (test_parse_range_header: False Negative, result == test_array[i].shouldPass);
+  mu_assert 

Re: [Bug-wget] Unit test case for parse_content_range()

[Ander Juaristi]

On 08/30/2015 05:57 PM, Darshit Shah wrote:

I've attached an updated patch with some test cases and a couple of
fixes to the parsing logic.



Hi,

The latest pull fails with the following error:

http.c: In function 'test_parse_range_header':
http.c:4933:3: error: 'for' loop initial declarations are only allowed in C99 
mode
   for (unsigned i = 0; i  countof (test_array); i++)
   ^
http.c:4933:3: note: use option -std=c99 or -std=gnu99 to compile your code

I ran './configure' without arguments (ie. default flags):

  Compiler:  gcc
  CFlags:-I/usr/include/p11-kit-1   -DHAVE_LIBGNUTLS   -DNDEBUG
  LDFlags:
  Libs:  -lgnutls   -lz
  SSL:   gnutls

Regards,
- AJ

[Bug-wget] Unit test case for parse_content_range()

[Darshit Shah]

I've written a unit test for the parse_content_range() method.
However, I haven't yet populated it with various test cases.
Sharing the patch for the unit test here. I will add more test cases
for this test later.

Kindly do review the patch. If no one complains, I'll push it in a
couple of days.

-- 
Thanking You,
Darshit Shah
From 154760a79b3981f8cb5fcf7f643ae2e2579aa887 Mon Sep 17 00:00:00 2001
From: Darshit Shah dar...@gmail.com
Date: Sat, 29 Aug 2015 23:08:39 +0530
Subject: [PATCH] Add unit test for parse_content_range() method

* http.c (test_parse_range_header): New function to test the
function for parsing the HTTP/1.1 Content-Range header.
* test.[ch]: Same
---
 src/http.c | 38 ++
 src/test.c |  1 +
 src/test.h |  1 +
 3 files changed, 40 insertions(+)

diff --git a/src/http.c b/src/http.c
index e96cad7..9bba036 100644
--- a/src/http.c
+++ b/src/http.c
@@ -4892,6 +4892,44 @@ ensure_extension (struct http_stat *hs, const char *ext, int *dt)
 }
 
 #ifdef TESTING
+
+const char *
+test_parse_range_header(void)
+{
+  static const struct {
+const char * rangehdr;
+const wgint firstbyte;
+const wgint lastbyte;
+const wgint length;
+  } test_array[] = {
+  { bytes 0-1000/1, 0, 1000, 1}
+  };
+
+  /* wgint *firstbyteptr = xmalloc(sizeof(wgint)); */
+  wgint firstbyteptr[sizeof(wgint)];
+  wgint *lastbyteptr = xmalloc(sizeof(wgint));
+  wgint *lengthptr = xmalloc(sizeof(wgint));
+  bool result;
+  for (unsigned i = 0; i  countof (test_array); i++)
+{
+  result = parse_content_range(test_array[0].rangehdr, firstbyteptr, lastbyteptr, lengthptr);
+#if 0
+  printf(%ld %ld, test_array[i].firstbyte, *firstbyteptr);
+  printf(%ld %ld, test_array[i].lastbyte, *lastbyteptr);
+  printf(%ld %ld, test_array[i].length, *lengthptr);
+#endif
+  mu_assert(test_parse_range_header: Parsing failed, result);
+  mu_assert(test_parse_range_header: Bad parse, test_array[i].firstbyte == *firstbyteptr 
+  test_array[i].lastbyte == *lastbyteptr 
+  test_array[i].length == *lengthptr);
+}
+
+  /* xfree(firstbyteptr); */
+  xfree(lastbyteptr);
+  xfree(lengthptr);
+  return NULL;
+}
+
 const char *
 test_parse_content_disposition(void)
 {
diff --git a/src/test.c b/src/test.c
index 5278925..cb01de3 100644
--- a/src/test.c
+++ b/src/test.c
@@ -54,6 +54,7 @@ all_tests(void)
   mu_run_test (test_has_key);
 #endif
   mu_run_test (test_parse_content_disposition);
+  mu_run_test (test_parse_range_header);
   mu_run_test (test_subdir_p);
   mu_run_test (test_dir_matches_p);
   mu_run_test (test_commands_sorted);
diff --git a/src/test.h b/src/test.h
index f74c162..4e0e1f2 100644
--- a/src/test.h
+++ b/src/test.h
@@ -48,6 +48,7 @@ const char *test_has_key (void);
 const char *test_find_key_value (void);
 const char *test_find_key_values (void);
 const char *test_parse_content_disposition(void);
+const char *test_parse_range_header(void);
 const char *test_commands_sorted(void);
 const char *test_cmd_spec_restrict_file_names(void);
 const char *test_is_robots_txt_url(void);
-- 
2.5.0

Re: [Bug-wget] Unit test case for parse_content_range()

[Tim Rühsen]

Am Samstag, 29. August 2015, 23:13:23 schrieb Darshit Shah:
 I've written a unit test for the parse_content_range() method.
 However, I haven't yet populated it with various test cases.
 Sharing the patch for the unit test here. I will add more test cases
 for this test later.
 
 Kindly do review the patch. If no one complains, I'll push it in a
 couple of days.

Hi Darshit,

some of the 'valid' tests

0-max
{ bytes 0-1000/1000, 0, 1000, 1000}
non0-max
{ bytes 1-1000/1000, 1, 1000, 1000}
0-valid
{ bytes 0-500/1000, 0, 500, 1000}
non0-valid
{ bytes 1-500/1000, 1, 500, 1000}
0-(max-1)
{ bytes 0-999/1000, 0, 999, 1000}
non0-(max-1)
{ bytes 1-999/1000, 1, 999, 1000}

And please add some tests using =2^31 and =2^32 as values.

Regards, Tim


signature.asc
Description: This is a digitally signed message part.

Re: [Bug-wget] Unit test case for parse_content_range()

[Darshit Shah]

On Sun, Aug 30, 2015 at 12:51 AM, Tim Rühsen tim.rueh...@gmx.de wrote:
 Am Samstag, 29. August 2015, 23:13:23 schrieb Darshit Shah:
 I've written a unit test for the parse_content_range() method.
 However, I haven't yet populated it with various test cases.
 Sharing the patch for the unit test here. I will add more test cases
 for this test later.

 Kindly do review the patch. If no one complains, I'll push it in a
 couple of days.

 Hi Darshit,

 some of the 'valid' tests

On closer inspection, some of these are *NOT* valid Header values. But
Wget currently passes them. This is a parsing bug in my opinion.

RFC 7233 states:

 A Content-Range field value is invalid if it contains a
   byte-range-resp that has a last-byte-pos value less than its
   first-byte-pos value, or a complete-length value less than or equal
   to its last-byte-pos value.  The recipient of an invalid
   Content-Range MUST NOT attempt to recombine the received content with
   a stored representation.

Based on this, the first two examples provided are illegal. Similarly
a header value such as:
{ bytes 100-99/1000, 100, 99, 1000}
should also be very clearly illegal, but Wget currently allows it. I'm
not sure about the behaviour of the program on receipt of such a
header, but the function should clearly be failing on this test.

 0-max
 { bytes 0-1000/1000, 0, 1000, 1000}
 non0-max
 { bytes 1-1000/1000, 1, 1000, 1000}
 0-valid
 { bytes 0-500/1000, 0, 500, 1000}
 non0-valid
 { bytes 1-500/1000, 1, 500, 1000}
 0-(max-1)
 { bytes 0-999/1000, 0, 999, 1000}
 non0-(max-1)
 { bytes 1-999/1000, 1, 999, 1000}

 And please add some tests using =2^31 and =2^32 as values.

 Regards, Tim



-- 
Thanking You,
Darshit Shah