This is the mail archive of the cygwin mailing list for the Cygwin project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: FW: buffer size calculation in gethostby_helper()

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

For  2.,  both   string_ptr and  string_size  should be untouched
for 1.,   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 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
          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)

  #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.


-- Problem reports: FAQ: Documentation: Unsubscribe info:

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]