Bug 8195 - rpc client code doesn't work against NT4, when we need to fragment requests
Summary: rpc client code doesn't work against NT4, when we need to fragment requests
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: DCE-RPCs and pipes (show other bugs)
Version: 3.6.0rc2
Hardware: All Windows NT
: P5 regression
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 7803
  Show dependency treegraph
 
Reported: 2011-05-31 15:52 UTC by Stefan Metzmacher
Modified: 2011-06-17 18:59 UTC (History)
5 users (show)

See Also:


Attachments
Possible patch for master (needs testing) (1.66 KB, patch)
2011-05-31 16:21 UTC, Stefan Metzmacher
no flags Details
Possible better patches for master (needs testing) (7.38 KB, patch)
2011-06-07 17:17 UTC, Stefan Metzmacher
no flags Details
Patch for the main problem (for v3-6) (7.69 KB, patch)
2011-06-09 18:25 UTC, Stefan Metzmacher
gd: review+
Details
Patch for the cli_trans and cli_read/write problems (for master) (30.59 KB, patch)
2011-06-09 18:29 UTC, Stefan Metzmacher
vl: review+
jra: review+
Details
Patch for the cli_trans and cli_read/write problems (for v3-6-test) (28.54 KB, patch)
2011-06-14 20:28 UTC, Stefan Metzmacher
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Metzmacher 2011-05-31 15:52:10 UTC
When we try a ChangePasswordUser3 against a NT4 DC, we generate a DCERPC
request PDU with 1216 bytes. We fragment this at the SMB layer into
a write request with 1024 bytes and a trans request with 192 bytes.

This works fine with Windows 2000 and higher and also with Samba,
but Windows NT4 returns NT_STATUS_PIPE_BUSY to the trans request.

We need some research for the real fix.
I guess we have to create a HACK to make it working at all...
Comment 1 Stefan Metzmacher 2011-05-31 16:21:51 UTC
Created attachment 6506 [details]
Possible patch for master (needs testing)

Günther, please test this patch against NT4, w2k and older samba versions
and send me captures. (can I upload your captures, which demostrate the bug?)

From reading the code this should work against samba, even against samba 2.0.0.
Comment 2 Stefan Metzmacher 2011-06-07 05:44:57 UTC
Günther any chance to get that tested for rc2?
Comment 3 Stefan Metzmacher 2011-06-07 17:17:16 UTC
Created attachment 6539 [details]
Possible better patches for master (needs testing)

Günther please test with this patch.

Please also test rpcclient with spoolss, as well as the wbinfo change user password against NT4 and others.

Please also use valgrind while testing.

The first patch (1/3) which only changes the buffer size from 1024 to 4280
should be enough to fix the problem.

But it would be ugly to have 8540 bytes just for buffer on a possible idle connection. As winbindd uses 3 ncacn_np_ip connections we would have 3*8540 bytes.

So we better try to use dynamic buffers instead of fixed size buffers (patch 2/3).

The 3rd patch (3/3) is trivial...

metze
Comment 4 Stefan Metzmacher 2011-06-07 19:32:41 UTC
I've done some tests, the change itself look good.
But there're bugs in the cli_trans_format() code,
I have started a fix, but I need to clean it up.
Comment 5 Stefan Metzmacher 2011-06-09 18:23:19 UTC
Marking this as a blocker for now. As it discovered more problems with our
cli_trans_send/recv code and other areas in libsmb/.

I'll upload possible fixes later
Comment 6 Stefan Metzmacher 2011-06-09 18:25:57 UTC
Created attachment 6552 [details]
Patch for the main problem (for v3-6)

This patch only fixes one half of the problem, it depends on the following patches
Comment 7 Stefan Metzmacher 2011-06-09 18:29:21 UTC
Created attachment 6553 [details]
Patch for the cli_trans and cli_read/write problems (for master)

Volker, if you're fine with this changes, please push them to master
and then attach the cherry-picked patches for v3-6-test to this bug,
thanks!
Comment 8 Stefan Metzmacher 2011-06-09 18:34:25 UTC
Comment on attachment 6553 [details]
Patch for the cli_trans and cli_read/write problems (for master)

Jeremy, it would be nice if you could also have a look at this. Most of the problem was triggered by the small max_xmit of NT4 (4356), but in theory the problems can happen with each [nt]trans[2][s] call.
Comment 9 Jeremy Allison 2011-06-10 00:27:44 UTC
Comment on attachment 6553 [details]
Patch for the cli_trans and cli_read/write problems (for master)

Ok, went through really carefully and this looks good to me. I'll do more testing tomorrow morning before I go on vacation).
Comment 10 Volker Lendecke 2011-06-10 10:13:28 UTC
Comment on attachment 6553 [details]
Patch for the cli_trans and cli_read/write problems (for master)

Looks very good to me. Thanks!
Comment 11 Jeremy Allison 2011-06-10 18:59:25 UTC
Both patches have been pushed to master. Re-assigning to Karolin for inclusion in 3.6.0.
Jeremy.
Comment 12 Stefan Metzmacher 2011-06-12 18:45:21 UTC
Hi Karo,

please run the following to pick the required patches from
"Patch for the cli_trans and cli_read/write problems (for master)"

git cherry-pick -x fdfb5e95fee67bb7~15..fdfb5e95fee67bb7~1
(you can add the bug reference with git commit --amend then:-)

Günther still needs to review the patches for the main problem...
Comment 13 Guenther Deschner 2011-06-14 10:01:29 UTC
Comment on attachment 6552 [details]
Patch for the main problem (for v3-6)

looks good, and tested.

winbind properly falls back with this patch:

Password change with chgpasswd_user3 failed with: NT_STATUS_RPC_PROCNUM_OUT_OF_RANGE, retrying chgpasswd_user2
Comment 14 Guenther Deschner 2011-06-14 10:03:00 UTC
Karolin, please add the first patch to 3-6-test, thanks!
Comment 15 Karolin Seeger 2011-06-14 18:37:07 UTC
(In reply to comment #14)
> Karolin, please add the first patch to 3-6-test, thanks!

Pushed to v3-6-test.
Comment 16 Karolin Seeger 2011-06-14 18:43:15 UTC
(In reply to comment #12)
> Hi Karo,
> 
> please run the following to pick the required patches from
> "Patch for the cli_trans and cli_read/write problems (for master)"
> 
> git cherry-pick -x fdfb5e95fee67bb7~15..fdfb5e95fee67bb7~1

user@host:/data/git/samba/v3-6-test> git cherry-pick -x fdfb5e95fee67bb7~15..fdfb5e95fee67bb7~1
fatal: Cannot find 'fdfb5e95fee67bb7~15..fdfb5e95fee67bb7~1'

I tried to cherry-pick the following commits manually:

49cdf171a5198495aead9ace43963e805331e20b
173fc258e443d97e4ea37f2bee99c21ad15ab484
a25936f1b1300a76b08a6bd435bd7ccc388279d5
3dd1ebd21ee99d130f6dd30326ddafe3f00a50d0
2ae565b681a6307886b888ee5b576c12916eb0db
428a86c92b5b35e28c7d6921f2999616cdc1bc20
6f7af1b0388d30c8a06c495713066b90ded00780
0a8fd50bd806e925a915c74cb86733481b2144f6
5146c9ba9df063d6611abe356f9262adb027b091
10bb088cf1e005fd047c09afcf6b5b8999d416fe
1dd24ac06a7472f53b06bc0aaa54cb22c8da0f78
5d06b2197b5fd95aaf0394d1bdba957bac6c3570
73128b7cc7f536f80072a19cb69527c53d9a6c2f
f0ba1afe5f7dbafaf22c3028864de0f3910f675f
fdfb5e95fee67bb7bb3942270031d9260e0505b0
 
The last one fails with conflicts.
Could you please add a patch, please?

Thanks!
Comment 17 Stefan Metzmacher 2011-06-14 20:28:14 UTC
Created attachment 6584 [details]
Patch for the cli_trans and cli_read/write problems (for v3-6-test)
Comment 18 Stefan Metzmacher 2011-06-16 18:02:09 UTC
=> Karolin
Comment 19 Karolin Seeger 2011-06-17 18:59:01 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!