Re: [PATCH] IpAdress truncates printed port that exceeds 9999

On Sun, 23 May 2010 16:56:27 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 05/01/2010 07:49 PM, Amos Jeffries wrote:
>> Alex Rousskov wrote:
>>> Fixed IpAddress port printing for ports higher than 9999:
>>> snprintf includes zero-terminator in its size limit, so 7
>>> rather than 6 bytes are needed to snprintf a colon followed
>>> by 5 port digits.
>>>
>>> Whether the bug has any runtime effects in the current code,
>>> I do not know, but I did waste a few hours following
>>> misleading debugging output.
>
>
>> +1. Please commit with tweak:
>
> Committed (r10494). Please port to v3.1 (the original patch may work).
>
>
>> MAX_IPSTRLEN definition needs +1 as well to prevent this introducing a
>> buffer overflow.
>
> ToURL() operates on a buffer of blen length (not necessarily
> MAX_IPSTRLEN) and already checks for overflows. I do not see how it can
> cause a buffer overflow even if MAX_IPSTRLEN is 0.
>
> If you are not worried about overflows in ToURL() but about MAX_IPSTRLEN
> being too small, current MAX_IPSTRLEN=75 is probably already more than
> any IP address can consume:
>
> IPv4: 22 (xxx.xxx.xxx.xxx:ppppp)
> IPv6: 1+45+1+7=54?
>
(http://stackoverflow.com/questions/166132/maximum-length-of-the-textual-representation-of-an-ipv6-address)
>
> Did I miss something?

ToURL and NtoA should be fine, as you say they are designed to work with
external buffers which could be small.
I'm more worried about the external code allocating those buffers based on
MAX_IPSTRLEN. Which if it was out would make them all truncate.

I seem to be was miss-remembering from when MAX_IPSTRLEN was 48. 75 is
okay.

Another look over the ToURL and children shows ToHostname assuming the
port and brackets will take 7 bytes. But that ignores the colon between
port and encoded IP. Fixed that now too.

Amos
Received on Mon May 24 2010 - 01:57:57 MDT