The default bug view has changed. See this FAQ.

crash after reading freed memory during referral chase

RESOLVED FIXED

Status

Directory
LDAP C SDK
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Ulf Weltman, Assigned: mcs)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
Created attachment 272611 [details] [diff] [review]
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.

Updated

10 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Updated

10 years ago
Attachment #272611 - Flags: review?(mcs)
(Assignee)

Comment 2

10 years ago
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+
(Reporter)

Comment 3

10 years ago
Created attachment 273024 [details] [diff] [review]
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().
Attachment #272611 - Attachment is obsolete: true
Attachment #273024 - Flags: review?(mcs)
(Assignee)

Comment 4

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

Looks good to me.
Attachment #273024 - Flags: review?(mcs) → review+

Comment 5

10 years ago
Created attachment 281552 [details]
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?
(Reporter)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.