Last Comment Bug 388398 - crash after reading freed memory during referral chase
: crash after reading freed memory during referral chase
Status: RESOLVED FIXED
:
Product: Directory
Classification: Components
Component: LDAP C SDK (show other bugs)
: other
: All All
: -- normal (vote)
: ---
Assigned To: Mark Smith (:mcs)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-17 00:19 PDT by Ulf Weltman
Modified: 2007-09-19 16:20 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch proposal (4.33 KB, patch)
2007-07-17 00:23 PDT, Ulf Weltman
mcs: review+
Details | Diff | Review
proposal with feedback implemented (6.06 KB, patch)
2007-07-19 13:56 PDT, Ulf Weltman
mcs: review+
Details | Diff | Review
cvs commit message (645 bytes, text/plain)
2007-09-19 15:14 PDT, Noriko Hosoi
no flags Details

Description Ulf Weltman 2007-07-17 00:19:33 PDT
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 2007-07-17 00:23:01 PDT
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.
Comment 2 Mark Smith (:mcs) 2007-07-19 13:05:18 PDT
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 2007-07-19 13:56:46 PDT
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().
Comment 4 Mark Smith (:mcs) 2007-07-19 17:46:37 PDT
Comment on attachment 273024 [details] [diff] [review]
proposal with feedback implemented

Looks good to me.
Comment 5 Noriko Hosoi 2007-09-19 15:14:21 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.