Bug 12892 - Printing via printer share completely broken?
Summary: Printing via printer share completely broken?
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Printing (show other bugs)
Version: 4.7.0rc1
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: printing-maintainers
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-10 10:41 UTC by Stefan Metzmacher
Modified: 2021-01-11 10:40 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Metzmacher 2017-07-10 10:41:21 UTC
In source3/printing/printspoolss.c print_spool_open() we
always overwrite fsp->conn->spoolss_pipe, which introduces a memory
leak.

While trying to fix the memory leaks and use
fsp->print_file->spoolss_pipe instead of fsp->conn->spoolss_pipe,
I noticed that we call spoolss_OpenPrinter() and spoolss_StartDocPrinter()
in print_spool_open().

In print_spool_write() we only write the given bytes to the tempfile.

In print_spool_end() we spoolss_ClosePrinter(), with a comment
that this implicitly calls spoolss_EndDocPrinter().

But we never seem to call spoolss_WritePrinter() with the content from
the tempfile anywhere.
Comment 1 Jeremy Allison 2017-07-10 22:06:29 UTC
I think it works. smb2_create calls print_spool_open().

Inside print_spool_open() we create a filename relative to the root of the print share path via and open it as a file descriptor using:


120         pf->filename = talloc_asprintf(pf, "%s/%sXXXXXX",
121                                         lp_path(talloc_tos(),
122                                                 SNUM(fsp->conn)),
123                                         PRINT_SPOOL_PREFIX);
124         if (!pf->filename) {
125                 status = NT_STATUS_NO_MEMORY;
126                 goto done;
127         }
128         errno = 0;
129         mask = umask(S_IRWXO | S_IRWXG);
130         fd = mkstemp(pf->filename);

pf->filename is set into the DocumentInfo struct and passed into  _spoolss_StartDocPrinter() here:

179         info1 = talloc(tmp_ctx, struct spoolss_DocumentInfo1);
180         if (info1 == NULL) {
181                 status = NT_STATUS_NO_MEMORY;
182                 goto done;
183         }
184         info1->document_name = pf->docname;
185         info1->output_file = pf->filename;
186         info1->datatype = "RAW";
187 
188         info_ctr.level = 1;
189         info_ctr.info.info1 = info1;
190 
191         status = dcerpc_spoolss_StartDocPrinter(b, tmp_ctx,
192                                                 &pf->handle,
193                                                 &info_ctr,
194                                                 &pf->jobid,
195                                                 &werr);

... and then sets up as the target for normal writes into the returned (fd) handle via:

212         /* setup a full fsp */
213         fsp->fsp_name = synthetic_smb_fname(fsp, pf->filename, NULL, NULL, 0);
214         if (fsp->fsp_name == NULL) {
215                 status = NT_STATUS_NO_MEMORY;
216                 goto done;
217         }
218 
219         if (sys_fstat(fd, &fsp->fsp_name->st, false) != 0) {
220                 status = map_nt_error_from_unix(errno);
221                 goto done;
222         }
223 
224         fsp->file_id = vfs_file_id_from_sbuf(fsp->conn, &fsp->fsp_name->st);
225         fsp->fh->fd = fd;


Inside _spoolss_StartDocPrinter() we have:


 5943         werr = print_job_start(p->session_info,
 5944                                p->msg_ctx,
 5945                                rhost,
 5946                                snum,
 5947                                info_1->document_name,
 5948                                info_1->output_file,
 5949                                Printer->devmode,
 5950                                &Printer->jobid);

info_1->output_file is the open file we'll be writing into via the fd stored in fsp->fh->fd = fd inside print_spool_open().

print_job_start() allocates the jobid in the db, then calls:

2872         /* we have a job entry - now create the spool file */
2873         werr = print_job_spool_file(snum, jobid, filename, &pjob);

where filename == info_1->output_file above:

print_job_spool_file() does the following - given filename==info1->output_file==output_file parameter below:

static WERROR print_job_spool_file(int snum, uint32_t jobid,
                                   const char *output_file,
                                   struct printjob *pjob)
{
        WERROR werr;
        SMB_STRUCT_STAT st;
        const char *path;
        int len;
        mode_t mask;

        /* if this file is within the printer path, it means that smbd
         * is spooling it and will pass us control when it is finished.
         * Verify that the file name is ok, within path, and it is
         * already already there */
        if (output_file) {
                path = lp_path(talloc_tos(), snum);
                len = strlen(path);
                if (strncmp(output_file, path, len) == 0 &&
                    (output_file[len - 1] == '/' || output_file[len] == '/')) {

                        /* verify path is not too long */
                        if (strlen(output_file) >= sizeof(pjob->filename)) {
                                return WERR_INVALID_NAME;
                        }

                        /* verify that the file exists */
                        if (sys_stat(output_file, &st, false) != 0) {
                                return WERR_INVALID_NAME;
                        }

                        fstrcpy(pjob->filename, output_file);

                        DEBUG(3, ("print_job_spool_file:"
                                  "External spooling activated\n"));

                        /* we do not open the file until spooling is done */
                        pjob->fd = -1;
                        pjob->status = PJOB_SMBD_SPOOLING;

                        return WERR_OK;
                }
        }
...

When the file is closed, the close code calls print_spool_end() -> _spoolss_EndDocPrinter() -> print_job_end() where we have:

2964         if (close_type == NORMAL_CLOSE || close_type == SHUTDOWN_CLOSE) {
2965                 if (pjob->status == PJOB_SMBD_SPOOLING) {
2966                         /* take over the file now, smbd is done */
2967                         if (sys_stat(pjob->filename, &sbuf, false) != 0) {
2968                                 status = map_nt_error_from_unix(errno);
2969                                 DEBUG(3, ("print_job_end: "
2970                                           "stat file failed for jobid %d\n",
2971                                           jobid));
2972                                 goto fail;
2973                         }
2974 
2975                         pjob->status = LPQ_SPOOLING;
2976 

pjob->filename contains the data written into the file via normal write calls. print_job_end() eventually passes over to the backend via:

3038         ret = (*(current_printif->job_submit))(snum, pjob,
3039                                                current_printif->type, lpq_cmd);

So I think this still works (haven't tested, just analysed the code paths :-).

Jeremy.
Comment 2 Stefan Metzmacher 2017-07-11 06:53:29 UTC
(In reply to Jeremy Allison from comment #1)

Thanks! That explains it, but it's horrible :-)

It seems that smbspool makes use of it, so we should be able to add
tests for it. Once we have tests we should be able to refactor the
code to use spoolss_WritePrinter() from smbd and avoid the shared
file hack.
Comment 4 Björn Jacke 2021-01-11 10:40:34 UTC
Andreas / printingmaintainers: what is the status of this?