Bug 6776 - Running overlapping Byte Lock test will core dump Samba daemon
Running overlapping Byte Lock test will core dump Samba daemon
Status: RESOLVED FIXED
Product: Samba 3.4
Classification: Unclassified
Component: File services
3.4.0
All Linux
: P3 critical
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-02 16:09 UTC by Barry Sabsevitz
Modified: 2009-11-30 13:58 UTC (History)
1 user (show)

See Also:


Attachments
Program to reproduce Samba core dump. (3.94 KB, text/plain)
2009-10-02 16:13 UTC, Barry Sabsevitz
no flags Details
Patch for master/v3-5-test (17.58 KB, patch)
2009-10-02 23:20 UTC, Jeremy Allison
no flags Details
git-am format patch for 3.4.x. (18.01 KB, patch)
2009-10-05 16:24 UTC, Jeremy Allison
vl: review+
Details
git-am format patch for 3.3.x. (17.17 KB, patch)
2009-10-05 18:31 UTC, Jeremy Allison
vl: review+
Details
git-am format patch for 3.2.x. (17.17 KB, patch)
2009-10-05 18:58 UTC, Jeremy Allison
vl: review+
Details
Git formatted patch for 3.0.x (17.35 KB, patch)
2009-10-06 13:42 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Barry Sabsevitz 2009-10-02 16:09:01 UTC
We found this bug running the locktest2 test program (source included below)
on Samba 3.4.0 as well as Samba 3.4.1 it
will cause the Samba daemon to core dump 100% of the time. 
The stack found in the smbd.log file is:

[2009/10/01 15:43:28,  8] locking/posix.c:189(posix_fcntl_lock)
*** glibc detected *** smbd: malloc(): memory corruption: 0x00002b4207b726c0 ***======= Backtrace: =========
/lib64/libc.so.6[0x2b41f150f4ac]
/lib64/libc.so.6(__libc_malloc+0x7a)[0x2b41f151102a]
/lib64/libc.so.6(vasprintf+0x3e)[0x2b41f15064ae]
smbd(dbgtext+0xa7)[0x2b41edd599c7]
smbd[0x2b41edd0d4de]
smbd(set_posix_lock_posix_flavour+0x131)[0x2b41edd0dc21]
smbd(brl_lock+0x45d)[0x2b41edd0bb1d]
smbd(do_lock+0x203)[0x2b41edd088d3]
smbd[0x2b41edb858b4]
smbd[0x2b41edb9252d]
smbd[0x2b41edb9405d]
smbd(reply_trans2+0x58f)[0x2b41edb975ef]
smbd[0x2b41edbb4c24]
smbd[0x2b41edbb7333]
smbd[0x2b41edbb76fd]
smbd(run_events+0x10f)[0x2b41edd7a9ef]
smbd(smbd_process+0x671)[0x2b41edbb6c11]

The root cause of the problem seems to be:

	 * The bug that exists is as follows (if you follow the code from
	 * brl_lock_posix() with the example below, you will see that duplicate
	 * locks are created in the lock_arr data structure with the same lock 
	 * type and range, which leads to much badness later.):
 	 *
	 * Assume we hold the following locks:
	 *	start: 80, len: 10, so we have range: 80->89 type: RD
	 *	start: 100, len: 20, so we have range: 100->119 type:WR
	 *
	 * Now we try to grab the lock at start:90, len:10, type: WR
	 * so we are trying to grab range:90->99 type:WR
	 *
	 * The function brl_lock_posix() will invoke the routine 
	 * brlock_posix_split_merge():
	 *	1. It compares the lock 90->99 (the lock we are grabbing) 
	 * 	to 80->89 (lock already held)
	 *	and sees that the lock types are different, so it can't 
	 *	coalesce them. It then decides to add 90->99 to the lock list 
	 * 	in the lck_arr variable without coalescing them, 
	 * 	so now the lck_arr list has:
	 * 	80->89 RD,
	 *	90->99 WR.
	 * This is ok.
	 *
 	 *	We return to brl_lock_posix() and it loops and invokes the 
	 * 	routine brlock_posix_split_merge() again. 
	 * 
	 *	We compare the lock 90->99 (what we are trying to grab) to 
	 * 	100->119 (already held) and the code decides 
	 * 	it can coalesce these locks as they are the same type and are
	 *	right next to each other (90->99 followed by 100->119). 
	 * So now lck_arr has: 
	 * 	80->89 RD,
	 *	90->99 WR, 
	 *	90->120 WR. 
	 *
 	 * This is bad! It has 2 locks duplicated in the list (namely
	 * 90->99 and 90->120, both of which are the same type of lock and span
	 * the same range 90->99).  The bug is that it had already decided to 
	 * add the lock range 90->99 WR but then still decides to coalesce it
	 * with 100->119, without altering the previous lock it added to
	 * the list. This leads to duplicate lock ranges added. 
	 *
	 * The code in brl_lock_posix() and brl_unlock_posix()  assumes that 
	 * lock_arr should never generate more than 2 additional locks 
	 * (worst case) as evidenced by the line of code that reads:
 	  tp = SMB_MALLOC_ARRAY(struct lock_struct, (br_lck->num_locks + 2));
	 * in brl_lock_posix(). But because of all the duplication going on in
	 * the above bug, we will end up splitting or coalescing locks that can
	 * in turn yield many more than 2 additional locks. This causes us to 
	 * overflow the end of the allocated buffer for tp and leads to a core 
	 * dump. With the right pattern of locks, which the test program below
	 * will invoke, we will overflow the allocation of tp by more than 2
	 * locks and core dump the samba daemon.
	 * Running the following program on 1 node to 1 share will reproduce
         * the problem. Run it as:
	 * 	./locktest2 -f <file name on CIFS mounted file system>
	 * It will hang for 10-20 seconds and then return. If you check the 
	 * state of the samba daemons, you will see that one has crashed.
         * The program will be attached to this defect as an attachment.
Comment 1 Barry Sabsevitz 2009-10-02 16:13:02 UTC
Created attachment 4777 [details]
Program to reproduce Samba core dump.

To compile the program, just download locktest2.c and execute "make locktest2". To run program, "./locktest2 -f <path to file on CIFS mount point> and it will hang for about 10-20 seconds and then exit. When it returns you will find the samba daemon has core dumped.
Comment 2 Jeremy Allison 2009-10-02 21:36:19 UTC
Ok, I have a patch for this. I'll test over the weekend...
Jeremy.
Comment 3 Jeremy Allison 2009-10-02 23:20:51 UTC
Created attachment 4778 [details]
Patch for master/v3-5-test

Here is the fix I'm testing over the weekend. Seems to work here. I'll leave the locktest2 program running overnight on the mounted cifs share, but it seems to be fine now. Please test and let me know.
Jeremy.
Comment 4 Jeremy Allison 2009-10-05 16:24:32 UTC
Created attachment 4785 [details]
git-am format patch for 3.4.x.

I think this needs to be in the next 3.4.x.
Jeremy.
Comment 5 Jeremy Allison 2009-10-05 18:31:05 UTC
Created attachment 4786 [details]
git-am format patch for 3.3.x.

git-am patch for 3.3.x.
Jeremy.
Comment 6 Jeremy Allison 2009-10-05 18:58:35 UTC
Created attachment 4787 [details]
git-am format patch for 3.2.x.

git-am format patch for 3.2.x.
Jeremy.
Comment 7 Jeremy Allison 2009-10-06 13:42:54 UTC
Created attachment 4790 [details]
Git formatted patch for 3.0.x
Comment 8 Jeremy Allison 2009-10-06 17:55:25 UTC
Karolin, once Volker or Metze have reviewed these patches please pull into the relevent (active) git trees (3.2.x and 3.0.x are not required).
Thanks !
Jeremy.
Comment 9 Volker Lendecke 2009-10-07 13:11:51 UTC
Comment on attachment 4786 [details]
git-am format patch for 3.3.x.

This is complex stuff.... While I did not really walk all code paths in every combination and I did not verify the semantics are still what posix expects, the logic looks simpler than in the old code.

This together with the fact that Jeremy promised to have run the stress tester over night --- put this in please :-)

Volker
Comment 10 Volker Lendecke 2009-10-07 13:14:29 UTC
Karolin, I would put the 3.3 one in. The others have the same logic (it seems :-)..

Volker
Comment 11 Karolin Seeger 2009-10-08 02:55:11 UTC
Pushed to v3-4-test and v3-3-test.
Closing out bug report.

Thanks!