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

[Will Dietz]

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?

Thanks,

~Will