[PATCH] Add support for APD_COPY_FROM_DIRECTORY in AddPrinterDriverEx()
Jeremy Allison
jra at samba.org
Thu Nov 17 23:30:11 UTC 2016
On Thu, Nov 17, 2016 at 09:04:13AM +0100, Andreas Schneider wrote:
>
> See attached patch :)
Thanks - much better.
But for new code I think we should check for talloc fails,
even in torture code. The new code in test_driver_copy_from_directory()
does a lot of unchecked CREATE_PRINTER_DRIVER_PATH() and
talloc_zero calls.
Can we fix that please (a simple 'goto fail' should do).
Don't want to give Coverity more things to complain
about.
If you're too busy I can fix up and submit - the
rest of the patch LGTM.
Cheers,
Jeremy.
> --
> Andreas Schneider GPG-ID: CC014E3D
> Samba Team asn at samba.org
> www.samba.org
> From 8bcf336ebcd7c60dd893d64abf743c22fdcd6aa6 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Tue, 15 Nov 2016 14:29:29 +0100
> Subject: [PATCH 1/3] s3:spoolss: Add support for COPY_FROM_DIRECTORY in
> AddPrinterDriverEx
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12415
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/include/nt_printing.h | 7 ++-
> source3/printing/nt_printing.c | 96 ++++++++++++++++++++++++-----
> source3/rpc_server/spoolss/srv_spoolss_nt.c | 18 ++++--
> 3 files changed, 100 insertions(+), 21 deletions(-)
>
> diff --git a/source3/include/nt_printing.h b/source3/include/nt_printing.h
> index e253658..e0003f9 100644
> --- a/source3/include/nt_printing.h
> +++ b/source3/include/nt_printing.h
> @@ -170,11 +170,14 @@ bool delete_driver_files(const struct auth_session_info *server_info,
> const struct spoolss_DriverInfo8 *r);
>
> WERROR move_driver_to_download_area(struct auth_session_info *session_info,
> - struct spoolss_AddDriverInfoCtr *r);
> + struct spoolss_AddDriverInfoCtr *r,
> + const char *driver_directory);
>
> WERROR clean_up_driver_struct(TALLOC_CTX *mem_ctx,
> struct auth_session_info *session_info,
> - struct spoolss_AddDriverInfoCtr *r);
> + struct spoolss_AddDriverInfoCtr *r,
> + uint32_t flags,
> + const char **driver_directory);
>
> void map_printer_permissions(struct security_descriptor *sd);
>
> diff --git a/source3/printing/nt_printing.c b/source3/printing/nt_printing.c
> index 334a56e..394a3e5 100644
> --- a/source3/printing/nt_printing.c
> +++ b/source3/printing/nt_printing.c
> @@ -864,7 +864,9 @@ static WERROR clean_up_driver_struct_level(TALLOC_CTX *mem_ctx,
> const char **config_file,
> const char **help_file,
> struct spoolss_StringArray *dependent_files,
> - enum spoolss_DriverOSVersion *version)
> + enum spoolss_DriverOSVersion *version,
> + uint32_t flags,
> + const char **driver_directory)
> {
> const char *short_architecture;
> int i;
> @@ -879,6 +881,43 @@ static WERROR clean_up_driver_struct_level(TALLOC_CTX *mem_ctx,
> return WERR_INVALID_PARAMETER;
> }
>
> + if (flags & APD_COPY_FROM_DIRECTORY) {
> + char *path;
> + char *q;
> +
> + /*
> + * driver_path is set to:
> + *
> + * \\PRINTSRV\print$\x64\{279245b0-a8bd-4431-bf6f-baee92ac15c0}\pscript5.dll
> + */
> + path = talloc_strdup(mem_ctx, *driver_path);
> + if (path == NULL) {
> + return WERR_NOT_ENOUGH_MEMORY;
> + }
> +
> + /* Remove pscript5.dll */
> + q = strrchr_m(path, '\\');
> + if (q == NULL) {
> + return WERR_INVALID_PARAMETER;
> + }
> + *q = '\0';
> +
> + /* Get \{279245b0-a8bd-4431-bf6f-baee92ac15c0} */
> + q = strrchr_m(path, '\\');
> + if (q == NULL) {
> + return WERR_INVALID_PARAMETER;
> + }
> +
> + /*
> + * Set driver_directory to:
> + *
> + * {279245b0-a8bd-4431-bf6f-baee92ac15c0}
> + *
> + * This is the directory where all the files have been uploaded
> + */
> + *driver_directory = q + 1;
> + }
> +
> /* clean up the driver name.
> * we can get .\driver.dll
> * or worse c:\windows\system\driver.dll !
> @@ -931,7 +970,9 @@ static WERROR clean_up_driver_struct_level(TALLOC_CTX *mem_ctx,
>
> WERROR clean_up_driver_struct(TALLOC_CTX *mem_ctx,
> struct auth_session_info *session_info,
> - struct spoolss_AddDriverInfoCtr *r)
> + struct spoolss_AddDriverInfoCtr *r,
> + uint32_t flags,
> + const char **driver_directory)
> {
> switch (r->level) {
> case 3:
> @@ -942,7 +983,9 @@ WERROR clean_up_driver_struct(TALLOC_CTX *mem_ctx,
> &r->info.info3->config_file,
> &r->info.info3->help_file,
> r->info.info3->dependent_files,
> - &r->info.info3->version);
> + &r->info.info3->version,
> + flags,
> + driver_directory);
> case 6:
> return clean_up_driver_struct_level(mem_ctx, session_info,
> r->info.info6->architecture,
> @@ -951,7 +994,9 @@ WERROR clean_up_driver_struct(TALLOC_CTX *mem_ctx,
> &r->info.info6->config_file,
> &r->info.info6->help_file,
> r->info.info6->dependent_files,
> - &r->info.info6->version);
> + &r->info.info6->version,
> + flags,
> + driver_directory);
> case 8:
> return clean_up_driver_struct_level(mem_ctx, session_info,
> r->info.info8->architecture,
> @@ -960,7 +1005,9 @@ WERROR clean_up_driver_struct(TALLOC_CTX *mem_ctx,
> &r->info.info8->config_file,
> &r->info.info8->help_file,
> r->info.info8->dependent_files,
> - &r->info.info8->version);
> + &r->info.info8->version,
> + flags,
> + driver_directory);
> default:
> return WERR_NOT_SUPPORTED;
> }
> @@ -1012,7 +1059,8 @@ static WERROR move_driver_file_to_download_area(TALLOC_CTX *mem_ctx,
> const char *driver_file,
> const char *short_architecture,
> uint32_t driver_version,
> - uint32_t version)
> + uint32_t version,
> + const char *driver_directory)
> {
> struct smb_filename *smb_fname_old = NULL;
> struct smb_filename *smb_fname_new = NULL;
> @@ -1021,9 +1069,21 @@ static WERROR move_driver_file_to_download_area(TALLOC_CTX *mem_ctx,
> NTSTATUS status;
> WERROR ret;
>
> - old_name = talloc_asprintf(mem_ctx, "%s/%s",
> - short_architecture, driver_file);
> - W_ERROR_HAVE_NO_MEMORY(old_name);
> + if (driver_directory != NULL) {
> + old_name = talloc_asprintf(mem_ctx,
> + "%s/%s/%s",
> + short_architecture,
> + driver_directory,
> + driver_file);
> + } else {
> + old_name = talloc_asprintf(mem_ctx,
> + "%s/%s",
> + short_architecture,
> + driver_file);
> + }
> + if (old_name == NULL) {
> + return WERR_NOT_ENOUGH_MEMORY;
> + }
>
> new_name = talloc_asprintf(mem_ctx, "%s/%d/%s",
> short_architecture, driver_version, driver_file);
> @@ -1076,7 +1136,8 @@ static WERROR move_driver_file_to_download_area(TALLOC_CTX *mem_ctx,
> }
>
> WERROR move_driver_to_download_area(struct auth_session_info *session_info,
> - struct spoolss_AddDriverInfoCtr *r)
> + struct spoolss_AddDriverInfoCtr *r,
> + const char *driver_directory)
> {
> struct spoolss_AddDriverInfo3 *driver;
> struct spoolss_AddDriverInfo3 converted_driver;
> @@ -1201,7 +1262,8 @@ WERROR move_driver_to_download_area(struct auth_session_info *session_info,
> driver->driver_path,
> short_architecture,
> driver->version,
> - ver);
> + ver,
> + driver_directory);
> if (!W_ERROR_IS_OK(err)) {
> goto err_exit;
> }
> @@ -1215,7 +1277,8 @@ WERROR move_driver_to_download_area(struct auth_session_info *session_info,
> driver->data_file,
> short_architecture,
> driver->version,
> - ver);
> + ver,
> + driver_directory);
> if (!W_ERROR_IS_OK(err)) {
> goto err_exit;
> }
> @@ -1231,7 +1294,8 @@ WERROR move_driver_to_download_area(struct auth_session_info *session_info,
> driver->config_file,
> short_architecture,
> driver->version,
> - ver);
> + ver,
> + driver_directory);
> if (!W_ERROR_IS_OK(err)) {
> goto err_exit;
> }
> @@ -1248,7 +1312,8 @@ WERROR move_driver_to_download_area(struct auth_session_info *session_info,
> driver->help_file,
> short_architecture,
> driver->version,
> - ver);
> + ver,
> + driver_directory);
> if (!W_ERROR_IS_OK(err)) {
> goto err_exit;
> }
> @@ -1273,7 +1338,8 @@ WERROR move_driver_to_download_area(struct auth_session_info *session_info,
> driver->dependent_files->string[i],
> short_architecture,
> driver->version,
> - ver);
> + ver,
> + driver_directory);
> if (!W_ERROR_IS_OK(err)) {
> goto err_exit;
> }
> diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c
> index 0f22b6f..9f7b87c 100644
> --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c
> +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c
> @@ -8551,7 +8551,9 @@ WERROR _spoolss_AddPrinterDriverEx(struct pipes_struct *p,
> {
> WERROR err = WERR_OK;
> const char *driver_name = NULL;
> + const char *driver_directory = NULL;
> uint32_t version;
> +
> /*
> * we only support the semantics of AddPrinterDriver()
> * i.e. only copy files that are newer than existing ones
> @@ -8561,7 +8563,8 @@ WERROR _spoolss_AddPrinterDriverEx(struct pipes_struct *p,
> return WERR_INVALID_PARAMETER;
> }
>
> - if (r->in.flags != APD_COPY_NEW_FILES) {
> + if (!(r->in.flags & APD_COPY_ALL_FILES) &&
> + !(r->in.flags & APD_COPY_NEW_FILES)) {
> return WERR_ACCESS_DENIED;
> }
>
> @@ -8575,12 +8578,19 @@ WERROR _spoolss_AddPrinterDriverEx(struct pipes_struct *p,
> }
>
> DEBUG(5,("Cleaning driver's information\n"));
> - err = clean_up_driver_struct(p->mem_ctx, p->session_info, r->in.info_ctr);
> - if (!W_ERROR_IS_OK(err))
> + err = clean_up_driver_struct(p->mem_ctx,
> + p->session_info,
> + r->in.info_ctr,
> + r->in.flags,
> + &driver_directory);
> + if (!W_ERROR_IS_OK(err)) {
> goto done;
> + }
>
> DEBUG(5,("Moving driver to final destination\n"));
> - err = move_driver_to_download_area(p->session_info, r->in.info_ctr);
> + err = move_driver_to_download_area(p->session_info,
> + r->in.info_ctr,
> + driver_directory);
> if (!W_ERROR_IS_OK(err)) {
> goto done;
> }
> --
> 2.10.2
>
>
> From 09f1f58e2f6f0a0e756c5e72d8932db48fea7287 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Tue, 15 Nov 2016 14:33:05 +0100
> Subject: [PATCH 2/3] s3:spoolss: Add some useful debug messages on error
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12415
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/rpc_server/spoolss/srv_spoolss_nt.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/source3/rpc_server/spoolss/srv_spoolss_nt.c b/source3/rpc_server/spoolss/srv_spoolss_nt.c
> index 9f7b87c..d4e568b 100644
> --- a/source3/rpc_server/spoolss/srv_spoolss_nt.c
> +++ b/source3/rpc_server/spoolss/srv_spoolss_nt.c
> @@ -8584,6 +8584,8 @@ WERROR _spoolss_AddPrinterDriverEx(struct pipes_struct *p,
> r->in.flags,
> &driver_directory);
> if (!W_ERROR_IS_OK(err)) {
> + DBG_ERR("clean_up_driver_struct failed - %s\n",
> + win_errstr(err));
> goto done;
> }
>
> @@ -8592,6 +8594,8 @@ WERROR _spoolss_AddPrinterDriverEx(struct pipes_struct *p,
> r->in.info_ctr,
> driver_directory);
> if (!W_ERROR_IS_OK(err)) {
> + DBG_ERR("move_driver_to_download_area failed - %s\n",
> + win_errstr(err));
> goto done;
> }
>
> @@ -8602,6 +8606,8 @@ WERROR _spoolss_AddPrinterDriverEx(struct pipes_struct *p,
> &driver_name,
> &version);
> if (!W_ERROR_IS_OK(err)) {
> + DBG_ERR("winreg_add_driver_internal failed - %s\n",
> + win_errstr(err));
> goto done;
> }
>
> --
> 2.10.2
>
>
> From 422539bcfb4b8398daaca7f34da3517fedd7540e Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Tue, 15 Nov 2016 18:34:22 +0100
> Subject: [PATCH 3/3] s4:torture: Add tortue test for AddPrinterDriverEx with
> COPY_FROM_DIRECTORY
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12415
>
> Pair-Programmed-With: Guenther Deschner <gd at samba.org>
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> Signed-off-by: Guenther Deschner <gd at samba.org>
> ---
> source4/torture/rpc/spoolss.c | 150 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 148 insertions(+), 2 deletions(-)
>
> diff --git a/source4/torture/rpc/spoolss.c b/source4/torture/rpc/spoolss.c
> index 5045c87..52bcb85 100644
> --- a/source4/torture/rpc/spoolss.c
> +++ b/source4/torture/rpc/spoolss.c
> @@ -53,6 +53,7 @@
> #define TORTURE_DRIVER_ADOBE_CUPSADDSMB "torture_driver_adobe_cupsaddsmb"
> #define TORTURE_DRIVER_TIMESTAMPS "torture_driver_timestamps"
> #define TORTURE_DRIVER_DELETER "torture_driver_deleter"
> +#define TORTURE_DRIVER_COPY_DIR "torture_driver_copy_from_directory"
> #define TORTURE_DRIVER_DELETERIN "torture_driver_deleterin"
> #define TORTURE_PRINTER_STATIC1 "print1"
>
> @@ -100,6 +101,7 @@ struct torture_driver_context {
> } local;
> struct {
> const char *driver_directory;
> + const char *driver_upload_directory;
> const char *environment;
> } remote;
> struct spoolss_AddDriverInfo8 info8;
> @@ -10327,6 +10329,38 @@ static const char *driver_directory_share(struct torture_context *tctx,
> return tok;
> }
>
> +#define CREATE_PRINTER_DRIVER_PATH(_d, _file) \
> + talloc_asprintf((_d), "%s\\%s\\%s", (_d)->remote.driver_directory, (_d)->remote.driver_upload_directory, (_file))
> +
> +
> +static bool create_printer_driver_directory(struct torture_context *tctx,
> + struct smbcli_state *cli,
> + struct torture_driver_context *d)
> +{
> + char *driver_dir;
> +
> + if (d->remote.driver_upload_directory == NULL) {
> + return true;
> + }
> +
> + driver_dir = talloc_asprintf(tctx,
> + "%s\\%s",
> + driver_directory_dir(d->remote.driver_directory),
> + d->remote.driver_upload_directory);
> +
> +
> + torture_comment(tctx,
> + "Create remote driver directory: %s\n",
> + driver_dir);
> +
> + torture_assert_ntstatus_ok(tctx,
> + smbcli_mkdir(cli->tree,
> + driver_dir),
> + "Failed to create driver directory");
> +
> + return true;
> +}
> +
> static bool upload_printer_driver_file(struct torture_context *tctx,
> struct smbcli_state *cli,
> struct torture_driver_context *d,
> @@ -10339,13 +10373,32 @@ static bool upload_printer_driver_file(struct torture_context *tctx,
> off_t nread = 0;
> size_t start = 0;
> const char *remote_dir = driver_directory_dir(d->remote.driver_directory);
> - const char *local_name = talloc_asprintf(tctx, "%s/%s", d->local.driver_directory, file_name);
> - const char *remote_name = talloc_asprintf(tctx, "%s\\%s", remote_dir, file_name);
> + const char *remote_name;
> + const char *local_name;
> + const char *p;
>
> if (!file_name || strlen(file_name) == 0) {
> return true;
> }
>
> + p = strrchr(file_name, '\\');
> + if (p == NULL) {
> + p = file_name;
> + } else {
> + p++;
> + }
> +
> + local_name = talloc_asprintf(tctx, "%s/%s", d->local.driver_directory, p);
> + if (d->remote.driver_upload_directory != NULL) {
> + remote_name = talloc_asprintf(tctx,
> + "%s\\%s\\%s",
> + remote_dir,
> + d->remote.driver_upload_directory,
> + p);
> + } else {
> + remote_name = talloc_asprintf(tctx, "%s\\%s", remote_dir, p);
> + }
> +
> torture_comment(tctx, "Uploading %s to %s\n", local_name, remote_name);
>
> fnum = smbcli_open(cli->tree, remote_name, O_RDWR|O_CREAT|O_TRUNC, DENY_NONE);
> @@ -10443,6 +10496,10 @@ static bool upload_printer_driver(struct torture_context *tctx,
> server_name, share_name);
>
> torture_assert(tctx,
> + create_printer_driver_directory(tctx, cli, d),
> + "failed to create driver directory");
> +
> + torture_assert(tctx,
> upload_printer_driver_file(tctx, cli, d, d->info8.driver_path),
> "failed to upload driver_path");
> torture_assert(tctx,
> @@ -10995,6 +11052,91 @@ static bool test_multiple_drivers(struct torture_context *tctx,
> return true;
> }
>
> +static bool test_driver_copy_from_directory(struct torture_context *tctx,
> + struct dcerpc_pipe *p)
> +{
> + struct torture_driver_context *d;
> + struct spoolss_StringArray *a;
> + uint32_t add_flags = APD_COPY_NEW_FILES|APD_COPY_FROM_DIRECTORY|APD_RETURN_BLOCKING_STATUS_CODE;
> + uint32_t delete_flags = DPD_DELETE_ALL_FILES;
> + struct dcerpc_binding_handle *b = p->binding_handle;
> + const char *server_name_slash = talloc_asprintf(tctx,
> + "\\\\%s",
> + dcerpc_server_name(p));
> + struct GUID guid = GUID_random();
> +
> + d = talloc_zero(tctx, struct torture_driver_context);
> +
> + d->local.environment =
> + talloc_asprintf(d, SPOOLSS_ARCHITECTURE_x64);
> +
> + d->local.driver_directory =
> + talloc_asprintf(d, "/usr/share/cups/drivers/x64");
> +
> + d->remote.driver_upload_directory = GUID_string2(d, &guid);
> +
> + torture_assert(tctx,
> + fillup_printserver_info(tctx, p, d),
> + "failed to fillup printserver info");
> +
> + d->ex = true;
> + d->info8.version = SPOOLSS_DRIVER_VERSION_200X;
> + d->info8.driver_name = TORTURE_DRIVER_COPY_DIR;
> + d->info8.architecture = d->local.environment;
> +
> + d->info8.driver_path = CREATE_PRINTER_DRIVER_PATH(d, "pscript5.dll");
> + d->info8.data_file = CREATE_PRINTER_DRIVER_PATH(d, "cups6.ppd");
> + d->info8.config_file = CREATE_PRINTER_DRIVER_PATH(d, "cupsui6.dll");
> + d->info8.help_file = CREATE_PRINTER_DRIVER_PATH(d, "pscript.hlp");
> +
> + a = talloc_zero(d, struct spoolss_StringArray);
> + a->string = talloc_zero_array(a, const char *, 3);
> + a->string[0] = CREATE_PRINTER_DRIVER_PATH(d, "cups6.inf");
> + a->string[1] = CREATE_PRINTER_DRIVER_PATH(d, "cups6.ini");
> +
> + d->info8.dependent_files = a;
> +
> + if (!directory_exist(d->local.driver_directory)) {
> + torture_skip(tctx,
> + "Skipping Printer Driver test as no local drivers "
> + "are available");
> + }
> +
> + torture_assert(tctx,
> + upload_printer_driver(tctx, dcerpc_server_name(p), d),
> + "failed to upload printer driver");
> +
> + torture_assert(tctx,
> + test_AddPrinterDriver_args_level_3(tctx,
> + b,
> + server_name_slash,
> + &d->info8,
> + add_flags,
> + true,
> + NULL),
> + "failed to add driver");
> +
> + torture_assert(tctx,
> + test_DeletePrinterDriverEx(tctx,
> + b,
> + server_name_slash,
> + d->info8.driver_name,
> + d->local.environment,
> + delete_flags,
> + d->info8.version),
> + "failed to delete driver");
> +
> + torture_assert(tctx,
> + check_printer_driver_files(tctx,
> + dcerpc_server_name(p),
> + d,
> + false),
> + "printer driver file check failed");
> +
> + talloc_free(d);
> + return true;
> +}
> +
> static bool test_del_driver_all_files(struct torture_context *tctx,
> struct dcerpc_pipe *p)
> {
> @@ -11187,6 +11329,10 @@ struct torture_suite *torture_rpc_spoolss_driver(TALLOC_CTX *mem_ctx)
>
> torture_rpc_tcase_add_test(tcase, "multiple_drivers", test_multiple_drivers);
>
> + torture_rpc_tcase_add_test(tcase,
> + "test_driver_copy_from_directory",
> + test_driver_copy_from_directory);
> +
> torture_rpc_tcase_add_test(tcase, "del_driver_all_files", test_del_driver_all_files);
>
> torture_rpc_tcase_add_test(tcase, "del_driver_unused_files", test_del_driver_unused_files);
> --
> 2.10.2
>
More information about the samba-technical
mailing list