[PATCH] Fix several memory and resource leaks
Andreas Schneider
asn at samba.org
Fri Aug 10 07:56:05 UTC 2018
On Thursday, 9 August 2018 21:01:31 CEST Jeremy Allison wrote:
> On Thu, Aug 09, 2018 at 11:16:08AM -0700, Jeremy Allison via samba-technical
wrote:
> > 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 :-).
>
> Just to be helpful, these are the ones that
> both Volker and I +1'ed (although you might
> want to look at his comments about re-arranging
> around the 'continue's in the last patch.
Attached is an updated patchset. There are 4 patches with need to be reviewed
again.
I've dropped one patch which was a false positive. I've reordered the patches
that the already reviewed patchsets are at the beginning. When I modified a
patch I removed the RB tags.
Please check again.
Thanks,
Andreas
--
Andreas Schneider asn at samba.org
Samba Team www.samba.org
GPG-ID: 8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D
-------------- next part --------------
>From 26eedf974ca3af23a5b15ea19d34e333d8d2c856 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 9 Aug 2018 15:53:45 +0200
Subject: [PATCH 1/9] wbinfo: Free memory when we leave wbinfo_dsgetdcname()
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>
Reviewed-by: Volker Lendecke <vl at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
nsswitch/wbinfo.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/nsswitch/wbinfo.c b/nsswitch/wbinfo.c
index 1b58c73602a..c456f6eb329 100644
--- a/nsswitch/wbinfo.c
+++ b/nsswitch/wbinfo.c
@@ -747,6 +747,9 @@ static bool wbinfo_dsgetdcname(const char *domain_name, uint32_t flags)
d_printf("%s\n", dc_info->dc_site_name);
d_printf("%s\n", dc_info->client_site_name);
+ wbcFreeMemory(str);
+ wbcFreeMemory(dc_info);
+
return true;
}
--
2.18.0
>From fed58af1a4325afb4d0672fe6610ae465dbd4d3b Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 9 Aug 2018 16:05:41 +0200
Subject: [PATCH 2/9] s3:passdb: Don't leak memory on error in fetch_ldap_pw()
Found by covscan.
A candidate to use tallac ...
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>
Reviewed-by: Volker Lendecke <vl at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
source3/passdb/secrets.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/source3/passdb/secrets.c b/source3/passdb/secrets.c
index 7533d6b842f..ce215b1f2b2 100644
--- a/source3/passdb/secrets.c
+++ b/source3/passdb/secrets.c
@@ -351,6 +351,8 @@ bool fetch_ldap_pw(char **dn, char** pw)
if (!old_style_key) {
DEBUG(0, ("fetch_ldap_pw: strdup failed!\n"));
+ SAFE_FREE(*pw);
+ SAFE_FREE(*dn);
return False;
}
@@ -361,6 +363,7 @@ bool fetch_ldap_pw(char **dn, char** pw)
if ((data == NULL) || (size < sizeof(old_style_pw))) {
DEBUG(0,("fetch_ldap_pw: neither ldap secret retrieved!\n"));
SAFE_FREE(old_style_key);
+ SAFE_FREE(*pw);
SAFE_FREE(*dn);
SAFE_FREE(data);
return False;
@@ -375,6 +378,7 @@ bool fetch_ldap_pw(char **dn, char** pw)
if (!secrets_store_ldap_pw(*dn, old_style_pw)) {
DEBUG(0,("fetch_ldap_pw: ldap secret could not be upgraded!\n"));
SAFE_FREE(old_style_key);
+ SAFE_FREE(*pw);
SAFE_FREE(*dn);
return False;
}
--
2.18.0
>From 74d95173c18e4fde6daa73148ca4e62ccf2ae736 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 9 Aug 2018 16:19:48 +0200
Subject: [PATCH 3/9] s3:utils: Do not overflow the destination buffer in
net_idmap_restore()
Found by covsan.
error[invalidScanfFormatWidth]: Width 128 given in format string (no. 2)
is larger than destination buffer 'sid_string[128]', use %127s to
prevent overflowing it.
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>
Reviewed-by: Volker Lendecke <vl at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
source3/utils/net_idmap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/source3/utils/net_idmap.c b/source3/utils/net_idmap.c
index fee8121aa60..4f365662a71 100644
--- a/source3/utils/net_idmap.c
+++ b/source3/utils/net_idmap.c
@@ -417,14 +417,14 @@ static int net_idmap_restore(struct net_context *c, int argc, const char **argv)
if ( (len > 0) && (line[len-1] == '\n') )
line[len-1] = '\0';
- if (sscanf(line, "GID %lu %128s", &idval, sid_string) == 2)
+ if (sscanf(line, "GID %lu %127s", &idval, sid_string) == 2)
{
ret = net_idmap_store_id_mapping(db, ID_TYPE_GID,
idval, sid_string);
if (ret != 0) {
break;
}
- } else if (sscanf(line, "UID %lu %128s", &idval, sid_string) == 2)
+ } else if (sscanf(line, "UID %lu %127s", &idval, sid_string) == 2)
{
ret = net_idmap_store_id_mapping(db, ID_TYPE_UID,
idval, sid_string);
--
2.18.0
>From fa17b18e556cf8c146775c93038e24c07acc37ac Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 9 Aug 2018 16:30:03 +0200
Subject: [PATCH 4/9] s3:utils: Do not leak memory in new_user()
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>
Reviewed-by: Volker Lendecke <vl at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
source3/utils/pdbedit.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/source3/utils/pdbedit.c b/source3/utils/pdbedit.c
index a353bae7c4e..5c947e2fbde 100644
--- a/source3/utils/pdbedit.c
+++ b/source3/utils/pdbedit.c
@@ -750,7 +750,7 @@ static int new_user(const char *username, const char *fullname,
NTSTATUS status;
struct dom_sid u_sid;
int flags;
- int ret;
+ int ret = -1;
tosctx = talloc_tos();
if (!tosctx) {
@@ -766,10 +766,14 @@ static int new_user(const char *username, const char *fullname,
}
pwd1 = get_pass( "new password:", stdin_get);
+ if (pwd1 == NULL) {
+ fprintf(stderr, "Failed to read passwords.\n");
+ goto done;
+ }
pwd2 = get_pass( "retype new password:", stdin_get);
- if (!pwd1 || !pwd2) {
+ if (pwd2 == NULL) {
fprintf(stderr, "Failed to read passwords.\n");
- return -1;
+ goto done;
}
ret = strcmp(pwd1, pwd2);
if (ret != 0) {
--
2.18.0
>From 29beeb6a850754286528f92e1b6f50ea9a8ae188 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 9 Aug 2018 16:38:49 +0200
Subject: [PATCH 5/9] s3:winbind: Fix memory leak in nss_init()
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>
Reviewed-by: Volker Lendecke <vl at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
---
source3/winbindd/nss_info.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/source3/winbindd/nss_info.c b/source3/winbindd/nss_info.c
index 473b1a3b66e..9c07b967fae 100644
--- a/source3/winbindd/nss_info.c
+++ b/source3/winbindd/nss_info.c
@@ -158,7 +158,7 @@ static NTSTATUS nss_init(const char **nss_list)
NTSTATUS status;
static bool nss_initialized = false;
int i;
- char *backend, *domain;
+ char *backend = NULL, *domain = NULL;
struct nss_function_entry *nss_backend;
/* check for previous successful initializations */
@@ -179,6 +179,9 @@ static NTSTATUS nss_init(const char **nss_list)
as necessary) */
for ( i=0; nss_list && nss_list[i]; i++ ) {
+ /* Loop cleanup, to not leak on 'continue' */
+ SAFE_FREE(backend);
+ SAFE_FREE(domain);
if ( !parse_nss_parm(nss_list[i], &backend, &domain) ) {
DEBUG(0,("nss_init: failed to parse \"%s\"!\n",
@@ -233,15 +236,15 @@ static NTSTATUS nss_init(const char **nss_list)
status = nss_domain_list_add_domain(domain, nss_backend);
if (!NT_STATUS_IS_OK(status)) {
+ SAFE_FREE(backend);
+ SAFE_FREE(domain);
return status;
}
-
- /* cleanup */
-
- SAFE_FREE( backend );
- SAFE_FREE( domain );
}
+ SAFE_FREE(backend);
+ SAFE_FREE(domain);
+
if ( !nss_domain_list ) {
DEBUG(3,("nss_init: no nss backends configured. "
"Defaulting to \"template\".\n"));
--
2.18.0
>From 9fa6c62bd6159f436834b2010918aa0b6e87418f 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 6/9] 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 | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/source4/lib/policy/gp_filesys.c b/source4/lib/policy/gp_filesys.c
index d48fc9fd6b0..267762dd27d 100644
--- a/source4/lib/policy/gp_filesys.c
+++ b/source4/lib/policy/gp_filesys.c
@@ -215,6 +215,7 @@ static NTSTATUS gp_get_file (struct smbcli_tree *tree, const char *remote_src,
fh_local = open(local_dst, O_WRONLY | O_CREAT | O_TRUNC, 0644);
if (fh_local == -1) {
DEBUG(0, ("Failed to open local file: %s\n", local_dst));
+ smbcli_close(tree, fh_remote);
return NT_STATUS_UNSUCCESSFUL;
}
@@ -224,11 +225,17 @@ 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)));
+ smbcli_close(tree, fh_remote);
+ 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) {
+ smbcli_close(tree, fh_remote);
+ close(fh_local);
+ return NT_STATUS_NO_MEMORY;
+ }
/* Copy the contents of the file */
while (1) {
@@ -240,27 +247,28 @@ 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"));
+ smbcli_close(tree, fh_remote);
+ close(fh_local);
talloc_free(buf);
return NT_STATUS_UNSUCCESSFUL;
}
nread += n;
}
+ /* Close the files */
+ smbcli_close(tree, fh_remote);
+ close(fh_local);
+
+ talloc_free(buf);
+
/* Bytes read should match the file size, or the copy was incomplete */
if (nread != file_size) {
DEBUG(0, ("Remote/local file size mismatch after copying file: "
"%s (remote %zu, local %zu).\n",
remote_src, file_size, nread));
- close(fh_local);
- talloc_free(buf);
return NT_STATUS_UNSUCCESSFUL;
}
- /* Close the files */
- smbcli_close(tree, fh_remote);
- close(fh_local);
-
- talloc_free(buf);
return NT_STATUS_OK;
}
--
2.18.0
>From 189e3e7b4bf158d6061159abb7ab09bdb05b32a5 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 7/9] 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 | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/source3/client/client.c b/source3/client/client.c
index f112b8c4ac1..25ba01d6216 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;
}
}
@@ -1193,6 +1197,9 @@ static int do_get(const char *rname, const char *lname_in, bool reget)
if (!NT_STATUS_IS_OK(status)) {
d_fprintf(stderr, "parallel_read returned %s\n",
nt_errstr(status));
+ if (newhandle) {
+ close(handle);
+ }
cli_close(targetcli, fnum);
return 1;
}
--
2.18.0
>From e9fb4c647cb2d53ee3dd02e8dae95068d53b034a Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 9 Aug 2018 16:02:16 +0200
Subject: [PATCH 8/9] s3:libads: Fix memory leaks in ads_krb5_chg_password()
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/libads/krb5_setpw.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/source3/libads/krb5_setpw.c b/source3/libads/krb5_setpw.c
index bc96ac603b1..0418fec5ad3 100644
--- a/source3/libads/krb5_setpw.c
+++ b/source3/libads/krb5_setpw.c
@@ -224,6 +224,7 @@ static ADS_STATUS ads_krb5_chg_password(const char *kdc_host,
krb5_get_init_creds_opt_free(context, opts);
krb5_free_context(context);
free(realm);
+ smb_krb5_free_addresses(context, addr);
DEBUG(1,("ads_krb5_chg_password: asprintf fail\n"));
return ADS_ERROR_NT(NT_STATUS_NO_MEMORY);
}
@@ -234,6 +235,7 @@ static ADS_STATUS ads_krb5_chg_password(const char *kdc_host,
kerb_prompter, NULL,
0, chpw_princ, opts);
krb5_get_init_creds_opt_free(context, opts);
+ smb_krb5_free_addresses(context, addr);
SAFE_FREE(chpw_princ);
SAFE_FREE(password);
--
2.18.0
>From 37681f3733e4af50f2ae7979b06fdb11f89f04bd Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 9 Aug 2018 16:15:10 +0200
Subject: [PATCH 9/9] s3:registry: Fix possible memory leak in
_reg_perfcount_multi_sz_from_tdb()
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/registry/reg_perfcount.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/source3/registry/reg_perfcount.c b/source3/registry/reg_perfcount.c
index db4451ecdeb..e31f8991642 100644
--- a/source3/registry/reg_perfcount.c
+++ b/source3/registry/reg_perfcount.c
@@ -168,6 +168,7 @@ static uint32_t _reg_perfcount_multi_sz_from_tdb(TDB_CONTEXT *tdb,
TDB_DATA kbuf, dbuf;
char temp[PERFCOUNT_MAX_LEN] = {0};
char *buf1 = *retbuf;
+ char *p = NULL;
uint32_t working_size = 0;
DATA_BLOB name_index, name;
bool ok;
@@ -185,13 +186,16 @@ static uint32_t _reg_perfcount_multi_sz_from_tdb(TDB_CONTEXT *tdb,
}
/* First encode the name_index */
working_size = (kbuf.dsize + 1)*sizeof(uint16_t);
- buf1 = (char *)SMB_REALLOC(buf1, buffer_size + working_size);
- if(!buf1) {
+ p = (char *)SMB_REALLOC(buf1, buffer_size + working_size);
+ if (p == NULL) {
+ SAFE_FREE(buf1);
buffer_size = 0;
return buffer_size;
}
+ buf1 = p;
ok = push_reg_sz(talloc_tos(), &name_index, (const char *)kbuf.dptr);
if (!ok) {
+ SAFE_FREE(buf1);
buffer_size = 0;
return buffer_size;
}
@@ -199,16 +203,19 @@ static uint32_t _reg_perfcount_multi_sz_from_tdb(TDB_CONTEXT *tdb,
buffer_size += working_size;
/* Now encode the actual name */
working_size = (dbuf.dsize + 1)*sizeof(uint16_t);
- buf1 = (char *)SMB_REALLOC(buf1, buffer_size + working_size);
- if(!buf1) {
+ p = (char *)SMB_REALLOC(buf1, buffer_size + working_size);
+ if (p == NULL) {
+ SAFE_FREE(buf1);
buffer_size = 0;
return buffer_size;
}
+ buf1 = p;
memset(temp, 0, sizeof(temp));
memcpy(temp, dbuf.dptr, dbuf.dsize);
SAFE_FREE(dbuf.dptr);
ok = push_reg_sz(talloc_tos(), &name, temp);
if (!ok) {
+ SAFE_FREE(buf1);
buffer_size = 0;
return buffer_size;
}
--
2.18.0
More information about the samba-technical
mailing list