[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Bug-wget] [PATCH] Fix --version wrapping issues and be more consistent
From: |
Will Dietz |
Subject: |
[Bug-wget] [PATCH] Fix --version wrapping issues and be more consistent with alignment |
Date: |
Fri, 9 Aug 2013 10:20:58 -0500 |
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
- [Bug-wget] [PATCH] Fix --version wrapping issues and be more consistent with alignment,
Will Dietz <=