bug-wget
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Will Dietz
Subject: Re: [Bug-wget] Review Request (Bug 39453)
Date: Thu, 8 Aug 2013 12:19:16 -0500

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).
* (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?

Thanks!

~Will

Attachment: 0001-format_and_print_line-Fix-bugs-when-wrapping-improve.patch
Description: Binary data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]