Samba 4.8 introduced new replacement %J, %j for pathsafe IP address. Unfortunatly this conflicts with with %J in "print command". All submitted print jobs get the client IP address as jobname. This happens because %J is implemented in talloc_sub_basic, which is called before the substitutions of "print command". Another clash is %p. This works, because it is implemented in talloc_sub_advanced, which is called after the substitutions of "print command". Other commands (lppause, lpresume use %j) may also be affected. Cheers Heinrich
Arg, this looks like a nasty regression from ca66efc24181ba6a7a4c13397af514b0972b4855 . Any thoughts on how we can resolve this? IMO we should probably add a special case handler for "print command", lppause and lpresume to ensure that printing specific replacements are handled there before the generic talloc_sub_basic() case.
Can you get me a debug level 10 log showing when the string in 'print command' is getting clobbered by the new call to talloc_sub_basic() ? I'm hoping we can differentiate by share type, or some other way of deciding this is a print share/string so we can protect the print substitutions.
Created attachment 14933 [details] Logleve 10 Log
Created attachment 14934 [details] smb config Comamnd was smbd -d 10 -s smb-x.conf
I've instrumented talloc_sub_basic()/%J with a call to log_stack_trace() with the following outcome (the share is used for generating PDFs): [2019/07/26 20:03:34.697354, 0] ../../lib/util/fault.c:261(log_stack_trace) BACKTRACE: 31 stack frames: #0 /usr/lib64/libsamba-util.so.0(log_stack_trace+0x30) [0x7f8067764460] #1 /usr/lib64/libsmbconf.so.0(talloc_sub_basic+0x4d1) [0x7f806624da01] #2 /usr/lib64/libsmbconf.so.0(lp_string+0x3a) [0x7f8066228bca] #3 /usr/lib64/samba/libsmbd-base-samba4.so(+0x9dfc4) [0x7f806726dfc4] #4 /usr/lib64/samba/libsmbd-base-samba4.so(print_job_end+0x38b) [0x7f806725392b] #5 /usr/lib64/samba/libsmbd-base-samba4.so(_spoolss_EndDocPrinter+0x7e) [0x7f80673ffe9e] #6 /usr/lib64/samba/libsmbd-base-samba4.so(+0x198403) [0x7f8067368403] #7 /usr/lib64/samba/libsmbd-base-samba4.so(+0x220572) [0x7f80673f0572] #8 /usr/lib64/samba/libsmbd-base-samba4.so(process_complete_pdu+0x1b65) [0x7f80673f2e95] #9 /usr/lib64/samba/libsmbd-base-samba4.so(named_pipe_packet_process+0xda) [0x7f80673aa42a] #10 /usr/lib64/libdcerpc-binding.so.0(+0xc8af) [0x7f805eb8d8af] #11 /usr/lib64/samba/libsamba-sockets-samba4.so(+0x7219) [0x7f8065fe9219] #12 /usr/lib64/samba/libsamba-sockets-samba4.so(+0x5fdb) [0x7f8065fe7fdb] #13 /usr/lib64/libtevent.so.0(tevent_common_invoke_immediate_handler+0x111) [0x7f8063a31e21] #14 /usr/lib64/libtevent.so.0(tevent_common_loop_immediate+0x1e) [0x7f8063a31e7e] #15 /usr/lib64/libtevent.so.0(+0xc77b) [0x7f8063a3777b] #16 /usr/lib64/libtevent.so.0(+0xace7) [0x7f8063a35ce7] #17 /usr/lib64/libtevent.so.0(_tevent_loop_once+0x9d) [0x7f8063a311dd] #18 /usr/lib64/libtevent.so.0(tevent_common_loop_wait+0x1b) [0x7f8063a313fb] #19 /usr/lib64/libtevent.so.0(+0xac87) [0x7f8063a35c87] #20 /usr/lib64/samba/libsmbd-base-samba4.so(smbd_process+0x76f) [0x7f806731285f] #21 /usr/sbin/smbd(+0xd970) [0x55e1c7857970] #22 /usr/lib64/libtevent.so.0(tevent_common_invoke_fd_handler+0x7f) [0x7f8063a319ff] #23 /usr/lib64/libtevent.so.0(+0xc94a) [0x7f8063a3794a] #24 /usr/lib64/libtevent.so.0(+0xace7) [0x7f8063a35ce7] #25 /usr/lib64/libtevent.so.0(_tevent_loop_once+0x9d) [0x7f8063a311dd] #26 /usr/lib64/libtevent.so.0(tevent_common_loop_wait+0x1b) [0x7f8063a313fb] #27 /usr/lib64/libtevent.so.0(+0xac87) [0x7f8063a35c87] #28 /usr/sbin/smbd(main+0x199d) [0x55e1c78529dd] #29 /lib64/libc.so.6(__libc_start_main+0xea) [0x7f8063691f8a] #30 /usr/sbin/smbd(_start+0x2a) [0x55e1c7852e5a] The relevant part of the stacktrace boils down to: talloc_sub_basic lp_string lp_print_command (direct jump to lp_string) generic_job_submit (via current_printif->job_submit) print_job_end There are a few ways to fix this: 1. rename the new and practically unused %j/%J to %k/%K (or any other convenient letter) and mention all currently implemented %-replacements in a single place in the documentation/code (eg. case 'J': /* print job name for print shares */ break; in talloc_sub_basic) 2. split talloc_sub_basic/lp_string/... into print/non-print (or sub/no_sub) versions 3. pass down snum/servicename to talloc_sub_basic For me there are a few reasons to go with 1.: - since the new %j/%J are only in distribution use for about half a year (Ubuntu 18.10, RHEL 7.6, SLES15SP1), nearly nobody will make use of it (I guess less than 10 people worldwide) - the "print" %J is nearly 20 years old and has many users (PDF printing shares) - the IP %j/%J may also be convenient for print shares - the revert/change can easily be backported (even to 4.8 since it's a kind of DoS for PDF shares) - maybe testparm could be changed to warn about %j/%J use in non-print shares I volunteer to implement a patch for 1. (maybe also for a different solution if it's not too invasive).
Another possible way would be to mark the printing related commands as constant="1" in their documentation that should avoid the lp_string() is called for them. Then we can do the printing related %j substitution first and then call lp_string() as a 2nd step. Yet another or additional way would be to replace it with something that doesn't require a single charater, similar to %$(envvar). Maybe %{jobid} %{jobname} %{clientip_pathsafe} %{serverip_pathsafe} %{printername}...
Created attachment 15487 [details] Tentative patch for the the suggestion in c#6 Something like the attached patch? Seems to work properly in the quick testing I did. Hopefully I have catched all the different lpq/lppause/lpresume/... incarnations.
(In reply to Franz Sirl from comment #7) In the meantime I did something similar here and noticed that the lprm command string is also used in print_queue_update(), which is visible in the log like: print_queue_update: Sending message -> printer = p, type = 13, lpq command = [/home/bbaumba/src/git/samba2/bin/vlp tdbfile\=/tmp/vlp.tdb lpq p] lprm command = [/home/bbaumba/src/git/samba2/bin/vlp tdbfile\=/tmp/vlp.tdb lprm p 192_168_4_76] Is does also work with the job-id substitution, which is replaced with the clients ip address here (192_168_4_76).
Created attachment 15504 [details] Tentative patch #2 (In reply to Björn Baumbach from comment #8) This should fix the problem you see. I'm not very satisfied with the naming talloc_sub_advanced_only(), but I didn't want to rename talloc_sub_advanced() yet. For the change in print_job_start() I'm not quite sure if it's better to call talloc_sub_advanced() or talloc_sub_advanced_only(). I believe the final result should be the same, but I couldn't figure or test the exact call sequence so far. At least I took the opportunity and removed the last user of standard_sub_advanced().
(In reply to Franz Sirl from comment #9) Thank you for updating the patch. There are some substitutions missing, now. I've tried some print jobs with your code changes and see in the printer queue, listed on the test client, that the print jobs are now owned by "%U" instead of the user name. talloc_sub_advanced_only() does at least not handle %U, which might be the reason here.
Created attachment 15516 [details] Tentative patch #3 (In reply to Björn Baumbach from comment #10) I see, so talloc_sub_advanced() really is needed. Should be fixed with this patch.
(In reply to Franz Sirl from comment #11) Franz, I've taken the liberty to apply your patch and split it into digestible commits. CI: https://gitlab.com/samba-team/samba/merge_requests/872 Patch: https://gitlab.com/samba-team/samba/merge_requests/872.patch Can you please test with this patchset if the fix still works? Thanks!
(In reply to Ralph Böhme from comment #12) Yes, your patch still works as expected for me. I applied the patch to 4.9 (just needed s/change_to_user_and_service/change_to_user/ in one place in service.c) and it built fine (see https://download.opensuse.org/repositories/home:/fsirl:/samba/SLE_15_SP1/ for the results). I hope the patch will also be backported to the active branches.
Created attachment 15613 [details] Patch for 4.10 and 4.11 backported from master
Karolin, could you please apply the patch to the relevant branches? Thanks!
(In reply to Andreas Schneider from comment #15) Pushed to autobuild-v4-{11,10}-test.
(In reply to Karolin Seeger from comment #16) Pushed to both branches. Closing out bug report. Thanks!