Bug 13417 - [smbspool] Commit a553f12 breaks argv handling from CUPS
[smbspool] Commit a553f12 breaks argv handling from CUPS
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Printing
4.8.1
All Linux
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-01 19:48 UTC by Devin J. Pohly
Modified: 2018-05-14 07:49 UTC (History)
4 users (show)

See Also:


Attachments
patch for master (4.65 KB, patch)
2018-05-03 08:18 UTC, Andreas Schneider
ddiss: review+
ab: review+
Details
patch for 4.8 (99 bytes, patch)
2018-05-08 06:14 UTC, Andreas Schneider
no flags Details
patch for 4.8 (6.71 KB, patch)
2018-05-08 06:15 UTC, Andreas Schneider
asn: review? (ddiss)
ab: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin J. Pohly 2018-05-01 19:48:04 UTC
Steps to reproduce:
1. Upgrade to Samba 4.8.x from 4.7.x.
2. Print to an SMB print URI.

Expected result:
Print success

Actual result:
smbspool prints a usage message and returns 1, causing print to fail.


Fix:
Revert commit a55ef12 [1], which appears to be intended as a cosmetic change to the usage error message, but which actually breaks the way smbspool handles arguments when called by CUPS as a backend (i.e. with the URI in argv[0]).

[1]: https://github.com/samba-team/samba/commit/a553f12418a16c58b278745e3da6329ce24fe3c7

I have tested this fix and printing via CUPS is restored.


Error log from CUPS:
D [30/Apr/2018:15:51:36 -0500] [Job 812] Usage: smb://myprintserver/myprintername [DEVICE_URI] job-id user title copies options [file]
D [30/Apr/2018:15:51:36 -0500] [Job 812] The DEVICE_URI environment variable can also contain the
D [30/Apr/2018:15:51:36 -0500] [Job 812] destination printer:
D [30/Apr/2018:15:51:36 -0500] [Job 812] smb://[username:password@][workgroup/]server[:port]/printer
D [30/Apr/2018:15:51:36 -0500] [Job 812] PID 11223 (/usr/lib/cups/backend/smb) stopped with status 1.
I [30/Apr/2018:15:51:36 -0500] [Job 812] Backend returned status 1 (failed)
D [30/Apr/2018:15:51:36 -0500] Discarding unused job-state-changed event...
I [30/Apr/2018:15:51:36 -0500] [Job 812] Printer stopped due to backend errors; please consult the error_log file for details.
Comment 1 David Disseldorp 2018-05-01 22:12:51 UTC
Looks like this is a pretty straightforward regression.
@Andreas: are you okay with the proposed revert here?
Comment 2 Andreas Schneider 2018-05-02 08:50:21 UTC
Which arguments is cups passing to smbspool?
Comment 3 David Disseldorp 2018-05-02 14:29:27 UTC
Downstream duplicate: https://bugzilla.suse.com/show_bug.cgi?id=1089278
Comment 4 Andreas Schneider 2018-05-03 07:35:28 UTC
The command is called like that:

D [02/May/2018:15:31:08 +0200] [Job 116] /usr/lib/cups/backend/smb 116 root Test Page 1 job-uuid=urn:uuid:a387858e-714d-3239-5ca8-ec9443c94039 job-originating-host-name=localhost date-time-at-creation= date-time-at-processing= time-at-creation=1525267867 time-at-processing=1525267867

This means it is called like that:

DEVICE_URI=smb://localhost/printer bin/smbspool 116 root "Test Page" 1 "job-uuid=urn:uuid:a387858e-714d-3239-5ca8-ec9443c94039 job-originating-host-name=localhost date-time-at-creation= date-time-at-processing= time-at-creation=1525267867 time-at-processing=1525267867"

So the argv calculations are wrong. I'm fixing it and writing tests.
Comment 5 Andreas Schneider 2018-05-03 08:18:17 UTC
Created attachment 14175 [details]
patch for master
Comment 6 Alexander Bokovoy 2018-05-03 08:26:25 UTC
Thanks. The patch looks good to me. I pushed it to autobuild
Comment 7 David Disseldorp 2018-05-03 08:45:52 UTC
djpohly: please confirm that Andreas's patch (https://bugzilla.samba.org/attachment.cgi?id=14175) works in your environment.
Comment 8 Devin J. Pohly 2018-05-03 21:08:24 UTC
I can verify smb:// printing works again with the patch applied to samba git.

Note that CUPS does pass the device URI as argv[0] (not argv[1]!) as well as in $DEVICE_URI, so don't expect the usage printf to display "smbspool" or any such sensible thing in the wild.  The backend(7) man page isn't clear as to whether backends are required to parse argv[0] or may rely on $DEVICE_URI always being set.
Comment 9 David Disseldorp 2018-05-03 22:14:54 UTC
Thanks a lot Devin, for your thorough bisect work in particular!
Comment 10 Andreas Schneider 2018-05-08 06:14:02 UTC
Created attachment 14182 [details]
patch for 4.8
Comment 11 Andreas Schneider 2018-05-08 06:15:38 UTC
Created attachment 14183 [details]
patch for 4.8
Comment 12 Alexander Bokovoy 2018-05-08 06:23:00 UTC
Comment on attachment 14183 [details]
patch for 4.8

LGTM.
Comment 13 Andreas Schneider 2018-05-08 15:54:12 UTC
Karolin, please add to 4.8. Thanks.
Comment 14 Karolin Seeger 2018-05-09 07:47:39 UTC
(In reply to Andreas Schneider from comment #13)
Pushed to autobuild-v4-8-test.
Comment 15 Karolin Seeger 2018-05-14 07:49:54 UTC
(In reply to Karolin Seeger from comment #14)
Pushed to v4-8-test.
Closing out bug report.

Thanks!