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

[Tim Ruehsen]

On Thursday 08 August 2013 12:19:16 Will Dietz wrote:
> On Thu, Aug 8, 2013 at 3:07 AM, Tim Ruehsen <address@hidden> wrote:
> > 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
> 
> Expanding the scope of the fix (I originally was only attempting to
> address the comparison bug), my latest suggested patch is attached,
> with the following highlights
> 
> * Fix comparison bug
> * No tabulation vs prefix-length bug (the issue you mention above,
> that could cause wrapping at the wrong point).
> * Avoid using strlen(prefix) for computing remaining characters (this
> is important to ensure proper behavior on different locales such as
> ja_JP.utf8).

Good point.

> * (Stylistic) Ensure consistent alignment by placing first line of
> text on 'second' line, indented.  This matches the style used for
> printing information about wgetrc and also makes reading the wrapped
> lines easier.
> * Replace dead code considering non-positive line_length with assert
> 
> Thoughts?

Very well.

You could slightly simplify your patch by leaving this line
        if (printf ("%s", prefix) < 0)
and initially set remaining_chars to -1.

Put Giuseppe (address@hidden) on CC and/or mark your posting as [PATCH].

Regards, Tim