[Gaim-patches] [ gaim-Patches-1162827 ] 64bit IPC dns child bug

[SourceForge.net <no...@so...>]

Patches item #1162827, was opened at 2005-03-14 03:27
Message generated for change (Comment added) made by chrono86
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=300235&aid=1162827&group_id=235

Category: None
Group: None
Status: Closed
Resolution: Accepted
Priority: 5
Submitted By: rian hunter (chrono86)
Assigned to: Mark Doliner (thekingant)
Summary: 64bit IPC dns child bug

Initial Comment:
In proxy.c the dns child sends an int, size_t, an array of chars, 
(more size_ts, and char arrays), then an int. The problem with this 
protocol is that in 64 bit UNIX implementations, size_t is 
implemented as an 8 byte integer, whereas int is still 4 bytes. This 
conflicts with the while loop in the client function host_resolved:

--- snip
		while(rc > 0) {
			rc=read(req->fd_out, &addrlen, sizeof(addrlen));
			if(rc>0 && addrlen > 0) {
				addr=g_malloc(addrlen);
				rc=read(req->fd_out, addr, addrlen);
				hosts = g_slist_append(hosts, 
GINT_TO_POINTER(addrlen));
				hosts = g_slist_append(hosts, addr);
			} else {
				break;
			}
		}
-- snip

The while loop depends on reading 8 bytes = 0 at a time to stop, but 
the dns child has been ending its request with only 4 bytes (the int). 
This patch declares a new constant in the gaim_dns_childthread 
function called "endzero" of type size_t (to match addrlen). The 
childthread now ends each dns request with endzero (8 bytes) 
instead of zero (4 bytes), so the client isn't waiting on the child 
forever.


----------------------------------------------------------------------

>Comment By: rian hunter (chrono86)
Date: 2005-03-21 02:57

Message:
Logged In: YES 
user_id=801658

Ah, the current file in CVS looks correct. Thanks a lot for
fixing this!

----------------------------------------------------------------------

Comment By: Mark Doliner (thekingant)
Date: 2005-03-20 14:43

Message:
Logged In: YES 
user_id=20979

Hmm, I have a feeling you weren't really looking at the
current file in CVS (there's a ~5 hour delay between
anonymous and developer CVS).

I changed it so that rc is always sent (as a 4 byte int),
and the size_t 0 is no longer sent before the char array. 
So the case where rc = 0 should look like this.

1. 4 Byte int, (rc)
2. 8 Byte size_t, * Byte char array, (repeat n times)
3. 8 Byte size_t (zero)

Or am I still missing something?

----------------------------------------------------------------------

Comment By: rian hunter (chrono86)
Date: 2005-03-20 14:34

Message:
Logged In: YES 
user_id=801658

Sorry but I just looked through your changes and what you
have in CVS still isnt' right either. I'll be very
descriptive. The important part of the host_resolved
function is from line 325 to line 316. This is the
transaction it expects:

1. Read an int (4 bytes) (might be an error code)
2. If int is zero (this is the case we are worried about)
and we just read more than zero bytes bytes then start the
address loop.
3. Read a size_t, then read size_t bytes, keep doing this
until the original size_t is 0.

So on a 64bit machine the transaction wants to be this:

1. 4 Byte int
2. 8 Byte size_t, * Byte char array (repeat n times)
3. stop when the size_t = 0.

But in the gaim_dns_childthread you chaned zero to a size_t,
this isn't entirely correct because if rc != 0 (line 482)
then the childthread sends an int error code, if not it
sends a zero size_t. This is inconsistent. In the case where
rc = 0, the transaction it sends is this:

1. 8 Byte size_t
2. 8 Byte size_t, * Byte char array, (repeat n times)
3. 8 Byte size_t

Do you see the inconsistency? As long the host_resolved
function is originally reading an int as the value to test
for success or not, you can't just send the first zero as a
size_t. That's why my patch is the way it is, it only ends
with a size_t.

----------------------------------------------------------------------

Comment By: Mark Doliner (thekingant)
Date: 2005-03-20 13:04

Message:
Logged In: YES 
user_id=20979

Ok, no, an int isn't right either.  I changed zero back to a
size_t.  The data sent from the child to the parent looks
like this:

int rc (regardless of whether rc is zero or non-zero)
size_t addrlen
char* addr
(repeat addrlen and char*)
size_t zero

----------------------------------------------------------------------

Comment By: Mark Doliner (thekingant)
Date: 2005-03-20 12:15

Message:
Logged In: YES 
user_id=20979

Good point, I didn't really look at the code the first time.
 Changing "zero" to an int should work.  I'll just do that,
instead.

----------------------------------------------------------------------

Comment By: rian hunter (chrono86)
Date: 2005-03-19 22:24

Message:
Logged In: YES 
user_id=801658

I don't think you can solve the problem by simply changing
zero to a size_t. If you look at the host_resolved function,
it reads a 4-byte error code that could also be the initial
zero (which if made to be a size_t would be 8 bytes on those
UNIX systems). That was the reason behind making a new
constant called endzero. If you make zero a size_t you
should play with the host_resolved function to make sure it
correlates.

----------------------------------------------------------------------

Comment By: Mark Doliner (thekingant)
Date: 2005-03-19 21:06

Message:
Logged In: YES 
user_id=20979

I just fixed this in oldstatus (1.2.1) and head (2.0).  I
ending up changing zero from an int to a size_t.  That
should work, right?  Thanks for tracking this down.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=300235&aid=1162827&group_id=235