[PATCH] Fix several memory and resource leaks

[Jeremy Allison]

On Thu, Aug 09, 2018 at 11:12:32AM -0700, Jeremy Allison via samba-technical wrote:
> 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.

Ah, Looks like Volker also had some good comments.

I won't push anything, I'll wait until you
address both his and my comments (some of
which were the same :-).

Feel free to push the ones that both Volker
and I reviewed without comment though :-).

Jeremy.