On Fri, Aug 9, 2013 at 2:05 AM, Tim Ruehsen <[email protected]> wrote: > On Thursday 08 August 2013 12:19:16 Will Dietz wrote: >> On Thu, Aug 8, 2013 at 3:07 AM, Tim Ruehsen <[email protected]> wrote: >> > On Wednesday 07 August 2013 17:37:43 Will Dietz wrote: >> >> On Wed, Aug 7, 2013 at 2:54 PM, Tim Rühsen <[email protected]> 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 ([email protected]) 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
