Closed Bug 388398 Opened 15 years ago Closed 15 years ago

crash after reading freed memory during referral chase

Categories

(Directory :: LDAP C SDK, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ulf, Assigned: mcs)

Details

Attachments

(2 files, 1 obsolete file)

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
Attached patch patch proposal (obsolete) — Splinter Review
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #272611 - Flags: review?(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!
Attachment #272611 - Flags: review?(mcs) → review+
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().
Attachment #272611 - Attachment is obsolete: true
Attachment #273024 - Flags: review?(mcs)
Comment on attachment 273024 [details] [diff] [review]
proposal with feedback implemented

Looks good to me.
Attachment #273024 - Flags: review?(mcs) → review+
Attached file 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?
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.