On Wed, Aug 7, 2013 at 2:54 PM, Tim Rühsen <tim.rueh...@gmx.de> 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? Thanks, ~Will