X-Recipient: archive-cygwin AT delorie DOT com
X-SWARE-Spam-Status: No, hits=-1.8 required=5.0	tests=BAYES_00,TW_CP
X-Spam-Check-By: sourceware.org
Message-ID: <4E458CAD.5050606@math.cas.cz>
Date: Fri, 12 Aug 2011 22:27:25 +0200
From: Jan Kolar <kolar AT math DOT cas DOT cz>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.9) Gecko/20100317 Thunderbird/3.0.4
MIME-Version: 1.0
To: "Pierre A. Humblet" <Pierre DOT Humblet AT ieee DOT org>
CC: cygwin AT cygwin DOT com
Subject: Re: FW: buffer size calculation in gethostby_helper()
References: <015301cc5909$6afc9e60$40f5db20$@ieee.org>
In-Reply-To: <015301cc5909$6afc9e60$40f5db20$@ieee.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
List-Id: <cygwin.cygwin.com>
List-Subscribe: <mailto:cygwin-subscribe AT cygwin DOT com>
List-Archive: <http://sourceware.org/ml/cygwin/>
List-Post: <mailto:cygwin AT cygwin DOT com>
List-Help: <mailto:cygwin-help AT cygwin DOT com>, <http://sourceware.org/ml/#faqs>
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com
Delivered-To: mailing list cygwin AT cygwin DOT com



My final understanding is this:

for each of  address_count  we use (and have to allocate)
1.  addrsize_out   of  bytes in the "string" area of buffer
2.  sizeof (char *)  of bytes in the h_addr_list area of buffer

Logically,
For  1.,  both   string_ptr and  string_size  should be untouched
while
for 2.,   both should be updated.


Conclusion:
This implies the buffer should be allocated as it is allocated,
while string_size should be changed to include  the value
of  address_count * addrsize_out.
Only so the code can be readable (so far as 1. remains true).
(I prefer readability since only the readable code is likely to be correct.)


Therefore I also think (but I am not 100% convinced) that, as much 
address_count is considered, the buffer usage is safe
and the debug message was wrong.

There might be more I doubt in the function.
We have to be careful since here the data come from outside.
Therefore I am willing to review the change -- instead of writing the 
patch myself.

Once you are with the code, please would you check the safety of
1122:   for (i = 0; i < >>>> ancount <<<<<
1122:   for (i = 0; i < ancount; i++, >>>>> ptr = curptr->data + ansize 
<<<<<<
1201:   ancount = alias_count + address_count; /* Valid records */
and other places before I start trying to understand it?


Best regards
Jan





On 12.8.2011 18:03, Pierre A. Humblet wrote:
> ...
> The initial logic seems to be OK: In the following statement
> sz = DWORD_round (sizeof(hostent))
>         + sizeof (char *) * (alias_count + address_count + 2)
>         + string_size
>         + address_count * addrsize_out;
> the incremented address_count generates two increases in sz:
> a chunk of size sizeof(char *) and another one of size addrsize_out.
> So the patch adding addrsize_out shouldn't be needed.
>    
Yes. Thus it seems that the buffer is properly allocated, good. However 
I got confused anyway:

>>> +          system_printf ("Note: JK hopping to fix the -4 bug in net.cc saying (if defed DEBUGGING) 'Please debug.' ");
>>>       }
>>>         /* Update the records */
>>>         curptr->type = antype; /* Host byte order */ @@ -1192,7
>>> +1194,7 @@ gethostby_helper (const char *name, cons
>>>             else
>>>           memcpy (string_ptr, curptr->data, addrsize_in);
>>>             string_ptr += addrsize_out;
>>> -          string_size -= addrsize_out;
>>> +          string_size -= addrsize_out; // jk-2011 FIXME BUG:   this makes it -4 sometimes - before my fix.
>>>        
> The bug is here: logically string_size shouldn't be decremented as it is used to account for name sizes, not for addresses.
> Note that at this point string_size is only used for debugging and the bug generates a false alarm.
>    
Yes, logically it shouldn't be decremented (but there is better logic 
explained above!). And yes, used for debugging only.
But then string_size and string_ptr are not a couple as I would expect.
My suggestion is above.
-------------------------------
> It's weird that it only shows up now.
> I see two ways of fixing it:
> 1) add string_size += addrsize_out; as in the patch but then adjust the computation of sz or
> 2)  remove the extraneous string_size -= addrsize_out and in the  #ifdef DEBUGGING below replace
>      if (string_size<  0)  by
>          if (string_ptr>  ((char *) ret) + sz)
>
>    
>>>             continue;
>>>           }
>>>   #ifdef DEBUGGING
>>>        
>> This looks basically correct to me, but the original code is not from me.
>> Pierre, would you mind to have a look?
>>      
>
> Sorry about that. I could fix it myself next week if desired.
>
> Pierre
>
>
>
>
>    


--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple