[PATCH] Fix several memory and resource leaks

[Jeremy Allison]

On Thu, Aug 09, 2018 at 04:53:58PM +0200, Andreas Schneider via samba-technical wrote:
> Hi,
> 
> please review the attached patch and push if OK.
> 
> 
> Thanks,
> 
> 
> 	Andreas
> 
> -- 
> Andreas Schneider                      asn at samba.org
> Samba Team                             www.samba.org
> GPG-ID:     8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D

> From 569a759a415fd29569b592c4debb90bfa3c894e5 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Thu, 9 Aug 2018 15:48:48 +0200
> Subject: [PATCH 01/10] libcli: Fix a possible memory leak in
>  smb1cli_req_writev_submit()
> 
> Found by covscan.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13567
> 
> Pair-Programmed-With: Justin Stephenson <jstephen at redhat.com>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> Signed-off-by: Justin Stephenson <jstephen at redhat.com>
> ---
>  libcli/smb/smbXcli_base.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c
> index ad1b67b8476..4023d0c0f13 100644
> --- a/libcli/smb/smbXcli_base.c
> +++ b/libcli/smb/smbXcli_base.c
> @@ -1748,6 +1748,7 @@ static NTSTATUS smb1cli_req_writev_submit(struct tevent_req *req,
>  		if (!NT_STATUS_IS_OK(status)) {
>  			DEBUG(0, ("Error in encrypting client message: %s\n",
>  				  nt_errstr(status)));
> +			SAFE_FREE(enc_buf);
>  			return status;
>  		}
>  		buf = (char *)talloc_memdup(state, enc_buf,

This also needs enc_buf to be set to NULL in the declaration
in order to be safe (what if common_encrypt_buffer() returns
an error without changing the value of enc_buf, it'll free
whatever random value was left on the stack.

> From 91bfe0c2fef41532ebbd1f0542fc58418a92bc20 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Thu, 9 Aug 2018 15:58:32 +0200
> Subject: [PATCH 03/10] s3:client: Avoid a possible fd leak in do_get()
> 
> Found by covscan.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13567
> 
> Pair-Programmed-With: Justin Stephenson <jstephen at redhat.com>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> Signed-off-by: Justin Stephenson <jstephen at redhat.com>
> ---
>  source3/client/client.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/source3/client/client.c b/source3/client/client.c
> index f112b8c4ac1..c1e55739b8d 100644
> --- a/source3/client/client.c
> +++ b/source3/client/client.c
> @@ -1160,6 +1160,7 @@ static int do_get(const char *rname, const char *lname_in, bool reget)
>  				start = lseek(handle, 0, SEEK_END);
>  				if (start == -1) {
>  					d_printf("Error seeking local file\n");
> +					close(handle);
>  					return 1;
>  				}
>  			}
> @@ -1181,6 +1182,9 @@ static int do_get(const char *rname, const char *lname_in, bool reget)
>  				      NULL);
>  		if(!NT_STATUS_IS_OK(status)) {
>  			d_printf("getattrib: %s\n", nt_errstr(status));
> +			if (newhandle) {
> +				close(handle);
> +			}
>  			return 1;
>  		}
>  	}

The above is incomplete, the same fix also needs adding to
the error path after the cli_pull() call.

> From 6c93412eb4f89f2197acdda96befa18710b3217f Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Thu, 9 Aug 2018 16:42:43 +0200
> Subject: [PATCH 10/10] s4:lib: Fix a possible fd leak in gp_get_file()
> 
> Found by covscan.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13567
> 
> Pair-Programmed-With: Justin Stephenson <jstephen at redhat.com>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> Signed-off-by: Justin Stephenson <jstephen at redhat.com>
> ---
>  source4/lib/policy/gp_filesys.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/source4/lib/policy/gp_filesys.c b/source4/lib/policy/gp_filesys.c
> index d48fc9fd6b0..700c91dccca 100644
> --- a/source4/lib/policy/gp_filesys.c
> +++ b/source4/lib/policy/gp_filesys.c
> @@ -224,11 +224,15 @@ static NTSTATUS gp_get_file (struct smbcli_tree *tree, const char *remote_src,
>  			NT_STATUS_IS_ERR(smbcli_getattrE(tree, fh_remote,
>  				&attr, &file_size, NULL, NULL, NULL))) {
>  		DEBUG(0, ("Failed to get remote file size: %s\n", smbcli_errstr(tree)));
> +		close(fh_local);
>  		return NT_STATUS_UNSUCCESSFUL;
>  	}
>  
>  	buf = talloc_zero_array(tree, uint8_t, buf_size);
> -	NT_STATUS_HAVE_NO_MEMORY(buf);
> +	if (buf == NULL) {
> +		close(fh_local);
> +		return NT_STATUS_NO_MEMORY;
> +	}
>  
>  	/* Copy the contents of the file */
>  	while (1) {
> @@ -241,6 +245,7 @@ static NTSTATUS gp_get_file (struct smbcli_tree *tree, const char *remote_src,
>  		if (write(fh_local, buf, n) != n) {
>  			DEBUG(0, ("Short write while copying file.\n"));
>  			talloc_free(buf);
> +			close(fh_local);
>  			return NT_STATUS_UNSUCCESSFUL;
>  		}
>  		nread += n;
> -- 

Incomplete, also needs to call smbcli_close() on
fh_remote in error cases.

Other patches look good - pushed. I'll update
these and re-post for you to examine.

Jeremy.