Hi, Samba4 (4.1.9) on Ubuntu 12.04 here (seen same behavior on 4.1.6 on Ubuntu 14.04). I've encountered an annoying bug with samba-tool, some exceptions aren't caught causing the tool to crash if it finds something it doesn't like in the smb.conf. I have a specific line in my conf : "include = /etc/samba/%U.smb.conf ", it's useful for applying specific samba configuration for a specific user, it works on the samba side, but when samba-tool reads the conf, it crashs : root@server:~/samba# samba-tool user list Processing section "[global]" (…) Can't find include file /etc/samba/%U.smb.conf params.c:pm_process() - Failed. Error returned from params.c:parse(). pm_process() returned No ERROR(runtime): uncaught exception - Unable to load default file File "/usr/lib/python2.7/dist-packages/samba/netcmd/__init__.py", line 175, in _run return self.run(*args, **kwargs) File "/usr/lib/python2.7/dist-packages/samba/netcmd/user.py", line 261, in run lp = sambaopts.get_loadparm() File "/usr/lib/python2.7/dist-packages/samba/getopt.py", line 92, in get_loadparm self._lp.load_default() In fact it crashes for any include with any variable substitution (%U, %g, %a…).
Created attachment 10116 [details] handle (ignore) substitution variable in smb.conf After diging into the code source i've done the following conclusion : The python code cannot handle nicely this exception because it calls C functions that only return true/false, if they work or not. I suggest the attached patch, tell me if it's fine. The function 'handle_include' in lib/param/loadparm.c returns false when the file it tries to read doesn't exist (which cause generic uncaught exception in python with samba-tool). I propose to search '%[a-zA-Z]' in the filename, if it doesn't exist, and return true (without processing the file and completing the parsed configuration).
This looks like a reasonable approach. Comments: - use strchr not strstr - follow README.Coding for style (I do realise much of this is poor style, but we try not to repeat our mistakes). In particular avoid CamalCase and use 'return true;' rather than 'return( true )' The history is that the 'source4' code was made strict on failure here. I like the compromise of only being strict when a % variable is involved. Andrew Bartlett
Created attachment 10170 [details] [fixed] handle (ignore) substitution variable in smb.conf Fixed patch in respect to the coding rules and A.Bartlett's advices.
Created attachment 10171 [details] [fixed] handle (ignore) substitution variable in smb.conf Did'nt upload the good one :)
Andrew, can you have a look at this? Thanks!
Created attachment 10257 [details] New patch for ignoring substitution vars I just fixed a rebase conflict and just tidied up just the minor things while I was at it. The patch itself seems good to me. Tested it and it works.
(In reply to comment #6) > Created attachment 10257 [details] > New patch for ignoring substitution vars > > I just fixed a rebase conflict and just tidied up just the minor things while I > was at it. > > The patch itself seems good to me. Tested it and it works. Indeed, made a mistake in the && and parenthesis and keeped a camel-cased variable, thanks !
Shouldn't the target milestone be set to 4.3 ?
Andrew, Garming: can you please push this to master and attach a backport patch for v4-3-test?
(In reply to Garming Sam from comment #6) As Michael Adam pointed out on IRC, this part is still not quite right: + if ((next_char && (next_char >= 'a' && next_char <= 'z')) + || (next_char >= 'A' && next_char <= 'Z')) { The A-Z test can occur when next_char is NULL.
Some comments: - I don't think Garmin should have changed the patch's authorship. It is still Quentin's patch. - The parenthesis in the if statement seems wrong: lower_check || upper_check should be grouped together - the check for next_char (!=0) is not needed - the DEBUG statement should be line-wrapped to meet 80 columns, as per README.Coding. I am in discussion with Quentin on irc, and he will provide an updated patch along these lines.
Created attachment 11420 [details] fix of the patch in regard to Douglas Bagnall and Michael Adam comments
Created attachment 11551 [details] cosmetics changes Just few cosmetics changes in regards to Michael Adam comment on github pullrequest page
Created attachment 11552 [details] cosmetics changes
Created attachment 11553 [details] adding fix comment format
Created attachment 11554 [details] merging the two patches
Created attachment 11555 [details] adding bug detail in commit message
This final patch looks good to me. Thanks Quentin for your patience! :-) Only one more question to Andrew: I was puzzled by the fact that the s4 code is much more strict regarding includes, since the s3 code uses this ability to run with includes to non-existing files (not only with substitutions) quite consciously. It is a feature used frequently. Hence my personal approach would be to rather be more generous towards failures of handle_include. Andrew: if you insist on that behaviour, I am fine to go with Quentin's current patch. Otherwise we can also re-discuss. Cheers - Michael
(In reply to Michael Adam from comment #18) I think consistency with the s3 code is the more important theme here. We shouldn't have two different sets of rules for different parts of the codebase. The history is that in the isolation from the real world after the s4 fork, it was thought that checking error codes and conditions should be done consistency, and that a incorrect smb.conf file should not load, rather than simply print a message and load regardless. That I think is a noble goal, but collides with the practical reality of how Samba is used, and what users expect.
LGTM - pushed. I'll upload a cherry-pick once this is in master.
Created attachment 11702 [details] git-am fix for 4.3.next, 4.2.next. Cherry-pick from master. Applies cleanly to 4.3.next, 4.2.next.
It looks like this was merged as 3c6ea3293c6aac67bc442f47185fd494714e4806, can this bug be closed? commit 3c6ea3293c6aac67bc442f47185fd494714e4806 Author: Quentin Gibeaux <qgibeaux@iris-tech.fr> Date: Thu Oct 29 13:48:27 2015 +0100 lib/param: handle (ignore) substitution variable in smb.conf BUG: https://bugzilla.samba.org/show_bug.cgi?id=10722 The function handle_include returns false when trying to include files that have a substitution variable in filename (like %U), this patch makes handle_include to ignore this case, to make samba-tool work when there is such include in samba's configuration. Error was : root@ubuntu:/usr/local/samba# grep 'include.*%U' etc/smb.conf include = %U.conf root@ubuntu:/usr/local/samba# ./bin/samba-tool user list Can't find include file %U.conf ERROR(runtime): uncaught exception - Unable to load default file Signed-off-by: Quentin Gibeaux <qgibeaux@iris-tech.fr> Reviewed-by: Michael Adam <obnox@samba.org> Reviewed-by: Jeremy Allison <jra@samba.org> Autobuild-User(master): Jeremy Allison <jra@samba.org> Autobuild-Date(master): Wed Dec 9 02:05:30 CET 2015 on sn-devel-104