Bug 10722 - samba-tool crashes with uncaught exception when parsing include = /path/to/%U.conf in smb.conf
samba-tool crashes with uncaught exception when parsing include = /path/to/%U...
Status: ASSIGNED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Python
4.1.9
All Linux
: P5 normal
: 4.3
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-07-17 07:37 UTC by Quentin Gibeaux
Modified: 2015-12-09 19:17 UTC (History)
5 users (show)

See Also:


Attachments
handle (ignore) substitution variable in smb.conf (976 bytes, patch)
2014-07-17 07:57 UTC, Quentin Gibeaux
no flags Details
[fixed] handle (ignore) substitution variable in smb.conf (976 bytes, patch)
2014-08-04 09:27 UTC, Quentin Gibeaux
no flags Details
[fixed] handle (ignore) substitution variable in smb.conf (1.19 KB, patch)
2014-08-04 09:30 UTC, Quentin Gibeaux
no flags Details
New patch for ignoring substitution vars (1.50 KB, patch)
2014-09-05 01:12 UTC, Garming Sam
no flags Details
fix of the patch in regard to Douglas Bagnall and Michael Adam comments (1.23 KB, patch)
2015-09-09 10:14 UTC, Quentin Gibeaux
qgibeaux: review+
Details
cosmetics changes (7.24 KB, patch)
2015-10-29 12:54 UTC, Quentin Gibeaux
no flags Details
cosmetics changes (2.03 KB, patch)
2015-10-29 12:57 UTC, Quentin Gibeaux
no flags Details
adding fix comment format (3.04 KB, patch)
2015-10-29 13:28 UTC, Quentin Gibeaux
no flags Details
merging the two patches (2.07 KB, patch)
2015-10-29 14:06 UTC, Quentin Gibeaux
no flags Details
adding bug detail in commit message (2.21 KB, patch)
2015-10-29 14:19 UTC, Quentin Gibeaux
no flags Details
git-am fix for 4.3.next, 4.2.next. (2.45 KB, patch)
2015-12-09 19:17 UTC, Jeremy Allison
jra: review? (obnox)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Quentin Gibeaux 2014-07-17 07:37:50 UTC
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…).
Comment 1 Quentin Gibeaux 2014-07-17 07:57:12 UTC
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).
Comment 2 Andrew Bartlett 2014-08-04 02:01:42 UTC
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
Comment 3 Quentin Gibeaux 2014-08-04 09:27:55 UTC
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.
Comment 4 Quentin Gibeaux 2014-08-04 09:30:20 UTC
Created attachment 10171 [details]
[fixed]  handle (ignore) substitution variable in smb.conf

Did'nt upload the good one :)
Comment 5 Stefan Metzmacher 2014-09-04 09:29:40 UTC
Andrew, can you have a look at this? Thanks!
Comment 6 Garming Sam 2014-09-05 01:12:58 UTC
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.
Comment 7 Quentin Gibeaux 2014-09-05 07:38:54 UTC
(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 !
Comment 8 Quentin Gibeaux 2015-08-05 11:54:12 UTC
Shouldn't the target milestone be set to 4.3 ?
Comment 9 Stefan Metzmacher 2015-09-02 08:28:35 UTC
Andrew, Garming: can you please push this to master and attach a backport patch
for v4-3-test?
Comment 10 Douglas Bagnall 2015-09-08 21:53:48 UTC
(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.
Comment 11 Michael Adam 2015-09-09 08:43:03 UTC
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.
Comment 12 Quentin Gibeaux 2015-09-09 10:14:09 UTC
Created attachment 11420 [details]
fix of the patch in regard to  Douglas Bagnall and Michael Adam comments
Comment 13 Quentin Gibeaux 2015-10-29 12:54:48 UTC
Created attachment 11551 [details]
cosmetics changes

Just few cosmetics changes in regards to Michael Adam comment on github pullrequest page
Comment 14 Quentin Gibeaux 2015-10-29 12:57:19 UTC
Created attachment 11552 [details]
cosmetics changes
Comment 15 Quentin Gibeaux 2015-10-29 13:28:52 UTC
Created attachment 11553 [details]
adding fix comment format
Comment 16 Quentin Gibeaux 2015-10-29 14:06:53 UTC
Created attachment 11554 [details]
merging the two patches
Comment 17 Quentin Gibeaux 2015-10-29 14:19:52 UTC
Created attachment 11555 [details]
adding bug detail in commit message
Comment 18 Michael Adam 2015-10-29 15:18:13 UTC
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
Comment 19 Andrew Bartlett 2015-10-29 21:51:34 UTC
(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.
Comment 20 Jeremy Allison 2015-12-08 22:01:59 UTC
LGTM - pushed. I'll upload a cherry-pick once this is in master.
Comment 21 Jeremy Allison 2015-12-09 19:17:55 UTC
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.