Due a hidden side-effect of find_service() function when adding a new share via MMC (Microsoft Console: rclick on My Computer-> Manage -> Action -> Connect to another computer-> Enter samba server name -> System tools -> rclick on Shares -> New share...) entered share name is distorted (converted to lowercase). In such view mixed-case share names are impossible (they are always lowercased). For example, when adding new share 'MyShareWithCapitalLetters' via MMC, the following command-line is running: [2010/11/08 13:05:12.022746, 10, pid=13966] rpc_server/srv_srvsvc_nt.c:2019(_srvsvc_NetShareAdd) _srvsvc_NetShareAdd: Running [./tools/add_share "smb.conf" "mysharewithcapitalletters" "/my_share" "Test share with capitals in the name" "0"] Attached patch can fix this problem: --- source3/rpc_server/srv_srvsvc_nt.c +++ source3/rpc_server/srv_srvsvc_nt.c @@ -1737,7 +1737,7 @@ WERROR _srvsvc_NetShareAdd(pipes_struct struct srvsvc_NetShareAdd *r) { char *command = NULL; - char *share_name = NULL; + char *share_name = NULL, *share_name_copy = NULL; char *comment = NULL; char *pathname = NULL; int type; @@ -1827,7 +1827,17 @@ WERROR _srvsvc_NetShareAdd(pipes_struct return WERR_ACCESS_DENIED; } - snum = find_service(share_name); + /* Dell 2010 / Samba upgrade project / MMC-related small fixes: + Before call to find_service make a copy of share_name + because find_service() modifies its parameter - + it converts share_name to to canonical form (all lowercase letters). + NB: find_service() converts share_name as a size-effect only if the service was not found; + so this fix is not applicable for _srvsvc_NetShareSetInfo because no translation there. */ + + if ((share_name_copy = talloc_strdup(ctx, share_name)) == NULL) { + return WERR_NOMEM; + } + snum = find_service(share_name_copy); /* Share already exists. */ if (snum >= 0) {
That patch seems to be wrong. find_service() takes a 'fstring' ( a 256byte char array as argument) not 'char *'. The caller should copy into a fstring on the stack and pass that one. There're are some other places in the code where we have that bug... Jeremy you instruced passing a dynamic string to find_service in commit acc0218f1396de2aad521dff722ada43631d92f9. metze
*** Bug 7782 has been marked as a duplicate of this bug. ***
Yep, strictly speaking you are right, but original code use the same approach (share_name is allocated by talloc_strdup). Also find_service itself uses dynamically-allocated string via SMB_STRDUP when trying to call to itself recursively (with default service). Does it really matter - fstring or char* ? It does if find_service() goes beyond the EOL (zero-termination char)?
I haven't looked at this details, but I hope Jeremy will:-)
Yes, there's a bug in that map_username uses fstrcpy to replace the username passed into it from find_service(). This can overwrite memory. It's not a security issue as map username is a root-only feature, but it could cause a crash. I'll fix this. Jeremy.
Created attachment 6054 [details] patch for 3.5.x. Can you test this patch please ? It should fix the problem you were trying to fix (losing the supplied case when adding a share) plus it should fix the underlying issue with find_service() potentially overwriting memory if map_username modifies the underlying name. Jeremy.
Yep, I've applied this patch and see that it's Ok (now _srvsvc_NetShareAdd uses original letters case - checked via logs). Thanks.
Comment on attachment 6054 [details] patch for 3.5.x. Volker or Michael, please review and assign to Karolin if you're happy. I think this is the right fix for 3.5.7. Jeremy.
(In reply to comment #8) > Comment on attachment 6054 [details] > patch for 3.5.x. > > Volker or Michael, please review and assign to Karolin if you're happy. I think > this is the right fix for 3.5.7. > Jeremy. Jeremy, this had slipped my attention - sorry! Should we reactivate this for 3.5.9 ? Is this still valid or fixed differently? The corresponding fix in master seems to be different. Cheers - Michael
Yes this patch is still good for 3.5.x. It's fixed via allocation in master/v3-6-test. Please +1 it and re-assign to Karolin to get it into 3.5.9. Jeremy.
Gosh I neglected "My Requests" list.. :-( I don't quite understand why this defect is set to fixed...
Created attachment 7928 [details] Jeremy's Patch turned to a "git format-patch" patch. I converted Jeremy's patch to git patch.
Sorry, I must have gotten distracted again... But now I now what confused me: After testing Jeremy's patch, Volodymyr seems to have set this bug to fixed. But bugs should only be set to fixed when the patch has made it into the release branch... Assigning to Karolin for inclusion into 3.5.$NEXT. Karo: The patch is missing the "review" flag, but my git format-patch version is effectively the "+" of Jeremy's original patch. Cheers - Michael
Pushed to v3-5-test. Closing out bug report. Thanks!
This patch seems to break 'make test'. Jeremy, can you have a look again? Thanks!
(In reply to comment #15) > This patch seems to break 'make test'. > Jeremy, can you have a look again? > > Thanks! Reverted patch for now. Please note that the next 3.5 release is scheduled for Monday, November 5.
I have figured out the problem that breaks "make test". With this patch we do: fstrcpy(service, service); but inside safe_strcpy() when in DEVELOPER mode we do: #ifdef DEVELOPER clobber_region(fn,line,dest, maxlength+1); #endif so we overwrite the src (as it's the same as dest here). Still thinking about the correct fix for this - should we allow safe_strcpy() to be safely called when src == dest ? Jeremy.
Created attachment 8128 [details] This additional patch fixes make test. I think this is probably something we need in master/3.6.x/4.0.x also. Jeremy.
(In reply to comment #18) > Created attachment 8128 [details] > This additional patch fixes make test. > > I think this is probably something we need in master/3.6.x/4.0.x also. > > Jeremy. Jeremy, can I go ahead with 3.5.19 without these patches and include them in the next bugfix release?
(In reply to comment #18) > Created attachment 8128 [details] > This additional patch fixes make test. This looks like a safe and reasonable shortcut anyways. I am just running make test on v3-5-test with the reinstated first patch and this patch currently to verify that there are no strange side effects (I can't think of any). > I think this is probably something we need in master/3.6.x/4.0.x also. In master this function is not present any more, nor is clobber_region.
Sure, they can wait until the release afterwards. Re-assigning to Karolin for inclusion in 3.5.20. Jeremy.
Comment on attachment 8128 [details] This additional patch fixes make test. verified the second patch per make test
Pushed both patches to v3-5-test. Closing out bug report. Thanks!