Bug 14481 - smbclient (and other utilities parsing smb.conf) fail to follow include directives
Summary: smbclient (and other utilities parsing smb.conf) fail to follow include direc...
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 4.12.6
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
Depends on:
Reported: 2020-09-07 12:36 UTC by Christian Ehrhardt
Modified: 2020-09-11 10:00 UTC (History)
1 user (show)

See Also:

RFC how the parsing can be solved by not skipping includes. (1.44 KB, patch)
2020-09-11 10:00 UTC, Christian Ehrhardt
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Ehrhardt 2020-09-07 12:36:35 UTC
while debugging an Ubuntu bug report (https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1893906) I realized that there might be an issue in the config parsing.

Include is defined as (man page)
"The file is included literally, as though typed in place." which isn't entirely true in all situations.

It turns out that the code goes through the same pm_process function when the smbd daemon or the smbclient tool is started. Yet the latter has the arg "global_only" set to true since the tool only needs the global configs.
Sounds right at first but the include statement - if appearing outside a [global] section - is ignored as well.

Due to that constructs like
some config
other config
include extra.conf

further global conf

Will only consider "further global conf" when initializing the server, but not even see any content of the include on the smbclient tool.

Moving the include up to the global section, or throwing in another [global] before the include helps to avoid the issue - but this isn't what user would expect I think.

Therefore I wanted to ask if the config parser should maybe better always follow include statements. Sure if there is no global config in there that can be ignored, but to realize that it needs to lead those extra files.

This might also be closed as "works as intended" (on the code side), but then I'd think the docs/man-pages should be updated to mention that.
Comment 1 Douglas Bagnall 2020-09-08 23:30:06 UTC
I don't think it is intentional.

Unfortunately [historical reasons] we have two different smb.conf parsing stacks, each with its own "include" handler. 

The documentation is tied to handle_include() in lib/param/loadparm.c.

smbclient is probably using lp_include() in source3/param/loadparm.c.

But I thought the fileserver would also be using source3/param/loadparm.c, so, well, I don't know.
Comment 2 Christian Ehrhardt 2020-09-09 05:58:35 UTC
Yeah from my debugging (seen ont he LP bug link) I can confirm that both use lp_include() in source3/param/loadparm.c.

The difference is that the clients use global_only=true which will make the parser ignore 'include' statements that are not in global sections.

My suggestion was to have the parser never ignore include statements (it can still ignore everything read from there if it isn't global).
Comment 3 Christian Ehrhardt 2020-09-11 09:58:40 UTC
The checks I mean do mostly the same, an early exit if:
  if (!bInGlobalSection && bGlobalOnly)

There are four such checks in the parsing:
- process_registry_globals
- lp_do_section
- process_smbconf_service
- do_parameter

From the logs it seems do_parameter is the interesting one here and it already has the param itself.
doing parameter include = /etc/samba/test1.conf

I have created a fix as a suggestion that I'll attach,
but there seem to be other places that are skipped due to global_only=true which cause ServicePtrs to be uninitialized and crashes if the patch is applied as-is.

I'd leave the details to the samba experts that know the inner-workings of this better, but the patch already shows how a solution could look like (just with rough edges left).
Comment 4 Christian Ehrhardt 2020-09-11 10:00:28 UTC
Created attachment 16221 [details]
RFC how the parsing can be solved by not skipping includes.

Attached an RFC patch how the parsing can be solved by not skipping includes.
TODO: missing implications to the initialization of ServicePtrs