388398 - crash after reading freed memory during referral chase

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, defect)

Description

[Ulf Weltman]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20070623 Firefox/2.0.0.4
Build Identifier: 

wait4msg() has a big loop which waits for messages on the connections associated with the LDAP handle. It calls on read1msg() when there is something to read on a connection and if read1msg() finds a referral, it'll open a new connection to chase it.  It may also close those connections.

wait4msg has a conditional statement just after read1msg() which accesses an int member of the connection structure it had passed to read1msg(), but it may be closed and freed by that time.  If that memory hasn't been allocated and used by then, all is fine, but if it has been written to then we may enter the conditional statement and try to dereference a pointer in the connection structure and seg fault.

Here's what it looks like in valgrind:
==5694== Invalid read of size 4
==5694==    at 0x402DA94: wait4msg (result.c:477)
==5694==    by 0x402C34C: nsldapi_result_nolock (result.c:144)
==5694==    by 0x402C145: ldap_result (result.c:111)
==5694==    by 0x4033A97: nsldapi_search_s (search.c:1004)
==5694==    by 0x4033918: ldap_search_s (search.c:938)
==5694==    by 0x8048741: main (reftest.c:51)
==5694==  Address 0x405CB4C is 36 bytes inside a block of size 48 free'd
==5694==    at 0x4004E41: free (vg_replace_malloc.c:235)
==5694==    by 0x40218B3: ldap_x_free (open.c:892)
==5694==    by 0x402A014: nsldapi_free_connection (request.c:848)
==5694==    by 0x402AA9F: nsldapi_free_request (request.c:1011)
==5694==    by 0x402AA58: nsldapi_free_request (request.c:1007)
==5694==    by 0x402E689: read1msg (result.c:763)
==5694==    by 0x402DA86: wait4msg (result.c:470)
==5694==    by 0x402C34C: nsldapi_result_nolock (result.c:144)
==5694==    by 0x402C145: ldap_result (result.c:111)
==5694==    by 0x4033A97: nsldapi_search_s (search.c:1004)
==5694==    by 0x4033918: ldap_search_s (search.c:938)
==5694==    by 0x8048741: main (reftest.c:51) 

Reproducible: Always

Comment 1

[Ulf Weltman]

Attachment: patch proposal

Change read1msg() to accept a pointer to a pointer to an LDAPConn so it can set it to NULL so wait4msg() will know it's no longer valid.

Comment 2

[Mark Smith [:mcs]]

Comment on attachment 272611 [details] [diff] [review]
patch proposal

>
> 		LDAP_MUTEX_LOCK( ld, LDAP_CONN_LOCK );
> 		LDAP_MUTEX_LOCK( ld, LDAP_REQ_LOCK );
> 		for ( lc = ld->ld_conns; lc != NULL; lc = lc->lconn_next ) {
> 			if ( lc->lconn_sb->sb_ber.ber_ptr <
> 			    lc->lconn_sb->sb_ber.ber_end ) {
> 				rc = read1msg( ld, msgid, all, lc->lconn_sb,
>-				    lc, result );
>+				    &lc, result );
>+				/* read1msg() may have closed the referral connection */
>+				if ( lc == NULL ) {
>+					lc = ld->ld_conns;
>+				}
> 				break;

Setting lc above is done only to indicate that a message was found,
right?  Maybe set a separate (new) flag instead?  I don't like setting
lc to ld->ld_conns, since that is not necessarily the connection the
received the message came from.

Also, please add comments near the calls to read1msg() and above the
definition of read1msg() to remind the reader that it may free the
connection (that was not obvious at all).

And thanks for finding and fixing this!

Comment 3

[Ulf Weltman]

Attachment: proposal with feedback implemented

Yeah, setting lc was only to make sure we don't go into the polling code block below it.  It seemed safe to set lc to the beginning of the connection list because it's not used after that point until it's reset again at the top, but it's certainly misleading and confusing.  I've replaced that with a flag to tell us whether we've read messages or need to poll.

Also added comments around read1msg().

Comment 4

[Mark Smith [:mcs]]

Comment on attachment 273024 [details] [diff] [review]
proposal with feedback implemented

Looks good to me.

Comment 5

[Noriko Hosoi]

Attachment: cvs commit message

Checked in the patch into HEAD with Ulf's permission.
> Applying the patch (attachment id=273024) by Ulf Weltman in Comment #3

Mark: could you change the bug status FIXED/RESOLVED?