[PATCH] Fix several memory and resource leaks
Volker Lendecke
Volker.Lendecke at SerNet.DE
Thu Aug 9 17:49:30 UTC 2018
On Thu, Aug 09, 2018 at 04:53:58PM +0200, Andreas Schneider via samba-technical wrote:
> please review the attached patch and push if OK.
Some RB+, some comments. Feel free to push the RB+ ones.
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
Meet us at Storage Developer Conference (SDC)
Santa Clara, CA USA, September 24th-27th 2018
-------------- next part --------------
From 503b898314bad768a7c1058a3c5c06145bdb66fa 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] VL: Isn't this a false positive? An instance of
Coverity not being able to understand NTSTATUS? Don't we need to also
NULL-initialize enc_buf to make sure Coverity isn't tricked into believing an
uninitialized pointer read?
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,
--
2.11.0
From f15a572916b95eac4a1db0b18dcc612b9470ca41 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 02/10] 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>
---
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.11.0
From 5943c3fbd241d9d0464d7de5fcec8f553ff5b14c 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>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
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;
}
}
--
2.11.0
From 4d7bfd1274ec71c0335b9d68795af3e279c6452c 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 04/10] VL: Is smb_krb5_addresses really free'd with a simple
SAFE_FREE? Isn't that an array of arrays?
s3:libads: Fix memory leak 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..5fbafdd93bb 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);
+ SAFE_FREE(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);
+ SAFE_FREE(addr);
SAFE_FREE(chpw_princ);
SAFE_FREE(password);
--
2.11.0
From 5137194da3232302ba4e4049dc617d285a012ebf 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 05/10] 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>
---
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.11.0
From 12d8a8ca56566545a88a6d7754141eaaf7ff409c 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 06/10] VL: Is this really correct? We do a SMB_REALLOC in line
189 already. If that fails, we unconditionally return without free'ing the
buffer, like in the patched REALLOC. At least this is not the same
everywhere.
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 | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/source3/registry/reg_perfcount.c b/source3/registry/reg_perfcount.c
index db4451ecdeb..a737d96d24e 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;
@@ -192,6 +193,7 @@ static uint32_t _reg_perfcount_multi_sz_from_tdb(TDB_CONTEXT *tdb,
}
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 +201,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.11.0
From 92863349715455a92711f46659bb384d56a69284 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 07/10] 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>
---
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.11.0
From 736626d2737d63a447993390d8882ba44c7efcd4 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 08/10] 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>
---
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 1871ed0b594..00efebd3663 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.11.0
From f5a9098189148528053305aa9505f550505f831c 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 09/10] VL: This looks correct, so RB+, although I'd prefer
them on every continue; statement. More lines, but less confusing.
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>
---
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.11.0
From b1340ab8d99792d041a791871cd07c1edd0b0892 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>
Reviewed-by: Volker Lendecke <vl at samba.org>
---
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;
--
2.11.0
More information about the samba-technical
mailing list