Re: [Bug-wget] Review Request (Bug 39453)

[Tim Ruehsen]

On Wednesday 07 August 2013 17:37:43 Will Dietz wrote:
> On Wed, Aug 7, 2013 at 2:54 PM, Tim Rühsen <address@hidden> wrote:
> > Am Mittwoch, 7. August 2013, 08:24:35 schrieb Will Dietz:
> >> Hi all,
> >> 
> >> There's a minor integer error in wget as described in the following bug
> >> report:
> >> 
> >> https://savannah.gnu.org/bugs/?39453
> >> 
> >> Patch is included, please review.
> >> 
> >> Thanks!
> > 
> > Hi Will,
> > 
> > isn't the real problem a signed/unsigned comparison ?
> > 
> > If remaining_chars becomes negative (due to token is longer or equal to
> > line_length), the comparison
> > 
> >       if (remaining_chars <= strlen (token))
> > 
> > is false or at least undefined.
> > 
> > If we change it to
> > 
> >       if (remaining_chars <= (int) strlen (token))
> > 
> > the function should work.
> > 
> > Using gcc -Wsign-compare warns about such constructs.
> > 
> > Isn't there another bug, when setting
> > 
> >         remaining_chars = line_length - TABULATION;
> > 
> > ?
> > 
> > line_length might already be without TABULATION:
> >   if (line_length <= 0)
> >   
> >     line_length = MAX_CHARS_PER_LINE - TABULATION;
> > 
> > Regards, Tim
> 
> Thanks for the response!
> 
> Yes, this is a signed/unsigned comparison error at its core.  In my
> proposed patch I chose to avoid letting 'remaining_chars' go negative
> in the first place in order to correctly handle tokens that required
> the full size_t to represent their length.  That said your suggested
> change is simpler and would also address the comparison issue.  This
> might be the way to go since such long tokens are at best very
> unlikely to occur if not impossible due to memory limits.
> 
> As for the second bug I'm not sure as the code would still print N
> characters for the first line and wrapped lines would be indented and
> contain TABULATION fewer characters before wrapping, which seems
> correct.  Whether or not 'MAX_CHARS_PER_LINE - TABULATION' is the
> correct value for N when line_length is zero or negative is not
> something I can comment on, but I see no reason to assume it is
> incorrect either.
> 
> Does this make sense?

The first line has no tabulation but a prefix. So, the length of 'prefix' 
should be taken into account instead of TABULATION.

The patch to handle both issues (signed/unsigned comparison and prefix length) 
IMHO should be:

diff --git a/src/main.c b/src/main.c
index 8ce0eb3..869e5db 100644
--- a/src/main.c
+++ b/src/main.c
@@ -844,11 +844,11 @@ format_and_print_line (const char *prefix, const char 
*line,
   line_dup = xstrdup (line);
 
   if (line_length <= 0)
-    line_length = MAX_CHARS_PER_LINE - TABULATION;
+    line_length = MAX_CHARS_PER_LINE;
 
   if (printf ("%s", prefix) < 0)
     return -1;
-  remaining_chars = line_length;
+  remaining_chars = line_length - strlen(prefix);
   /* We break on spaces. */
   token = strtok (line_dup, " ");
   while (token != NULL)
@@ -856,7 +856,7 @@ format_and_print_line (const char *prefix, const char 
*line,
       /* If however a token is much larger than the maximum
          line length, all bets are off and we simply print the
          token on the next line. */
-      if (remaining_chars <= strlen (token))
+      if (remaining_chars <= (int) strlen (token))
         {
           if (printf ("\n%*c", TABULATION, ' ') < 0)
             return -1;

Do you agree ?

Regards, Tim