[Bug-wget] [PATCH] Fix --version wrapping issues and be more consistent with alignment

[Will Dietz]

On Fri, Aug 9, 2013 at 2:05 AM, Tim Ruehsen <address@hidden> wrote:
> 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

Thanks for the feedback, Tim.

Updated patch attached.

Thanks!

~Will

0001-Fix-version-wrapping-issues-and-be-more-consistent-w.patch
Description: Binary data