Bug 13745 - print command %J substitution
Summary: print command %J substitution
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Printing (show other bugs)
Version: 4.8.3
Hardware: All All
: P5 regression (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-15 11:22 UTC by Heinrich Mislik
Modified: 2020-12-11 12:14 UTC (History)
8 users (show)

See Also:


Attachments
Logleve 10 Log (878.60 KB, text/x-log)
2019-03-15 13:25 UTC, Heinrich Mislik
no flags Details
smb config (322 bytes, text/plain)
2019-03-15 13:26 UTC, Heinrich Mislik
no flags Details
Tentative patch for the the suggestion in c#6 (8.38 KB, patch)
2019-09-24 08:29 UTC, Franz Sirl
no flags Details
Tentative patch #2 (13.37 KB, patch)
2019-10-01 09:15 UTC, Franz Sirl
no flags Details
Tentative patch #3 (13.36 KB, patch)
2019-10-07 12:49 UTC, Franz Sirl
no flags Details
Patch for 4.10 and 4.11 backported from master (30.30 KB, patch)
2019-11-07 20:03 UTC, Ralph Böhme
slow: review? (metze)
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Heinrich Mislik 2019-01-15 11:22:46 UTC
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
Comment 1 David Disseldorp 2019-01-15 13:52:14 UTC
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.
Comment 2 Jeremy Allison 2019-01-16 00:57:53 UTC
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.
Comment 3 Heinrich Mislik 2019-03-15 13:25:37 UTC
Created attachment 14933 [details]
Logleve 10 Log
Comment 4 Heinrich Mislik 2019-03-15 13:26:48 UTC
Created attachment 14934 [details]
smb config

Comamnd was smbd -d 10 -s smb-x.conf
Comment 5 Franz Sirl 2019-07-29 08:41:15 UTC
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).
Comment 6 Stefan Metzmacher 2019-09-12 12:56:51 UTC
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}...
Comment 7 Franz Sirl 2019-09-24 08:29:05 UTC
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.
Comment 8 Björn Baumbach 2019-09-30 14:05:56 UTC
(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).
Comment 9 Franz Sirl 2019-10-01 09:15:55 UTC
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().
Comment 10 Björn Baumbach 2019-10-02 11:18:43 UTC
(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.
Comment 11 Franz Sirl 2019-10-07 12:49:29 UTC
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.
Comment 12 Ralph Böhme 2019-10-31 11:52:26 UTC
(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!
Comment 13 Franz Sirl 2019-11-03 15:27:09 UTC
(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.
Comment 14 Ralph Böhme 2019-11-07 20:03:38 UTC
Created attachment 15613 [details]
Patch for 4.10 and 4.11 backported from master
Comment 15 Andreas Schneider 2019-11-12 11:26:33 UTC
Karolin, could you please apply the patch to the relevant branches? Thanks!
Comment 16 Karolin Seeger 2019-11-13 07:35:19 UTC
(In reply to Andreas Schneider from comment #15)
Pushed to autobuild-v4-{11,10}-test.
Comment 17 Karolin Seeger 2019-11-19 08:47:32 UTC
(In reply to Karolin Seeger from comment #16)
Pushed to both branches.
Closing out bug report.

Thanks!