[PATCH] Fix smbtorture memory leaks
Andreas Schneider
asn at samba.org
Wed May 16 12:54:16 UTC 2018
On Wednesday, 16 May 2018 14:29:19 CEST David Disseldorp via samba-technical
wrote:
> On Wed, 16 May 2018 13:36:39 +0200, David Disseldorp via samba-technical
wrote:
> > On Wed, 16 May 2018 12:14:11 +0200, Andreas Schneider via samba-technical
wrote:
> > > - torture_assert_int_equal(tctx, smbc_get ##option(ctx), val, "failed
"
> > > #option); + torture_assert_int_equal_goto(tctx, smbc_get
##option(ctx),
> > > val, ok, done, "failed " #option);> >
> > > #define TEST_OPTION_STRING(option, val) \
> > >
> > > torture_comment(tctx, "Testing smbc_set" #option "\n");\
> > > smbc_set ##option(ctx, strdup(val));\
>
> The strdup() leak here needs to be fixed too. The comment above
> justifying the strdup() is wrong as of
> 2d41b1ab78639abe4ae030ff482573f464564dd7.
>
> > > torture_comment(tctx, "Testing smbc_get" #option "\n");\
> > >
> > > - torture_assert_str_equal(tctx, smbc_get ##option(ctx), val, "failed
"
> > > #option); + torture_assert_str_equal_goto(tctx, smbc_get
##option(ctx),
> > > val, ok, done, "failed " #option);>
> > Gah, I know this is just test code, but please don't add extra macros
> > that use magic labels and variables. My preference would be to just
> > move the torture_assert out of the TEST_OPTION_X() macros and into the
> > caller.
>
> Hmm, the macros also magically use "tctx" and "ctx". I'd just drop
> them altogether.
See attached updated patchset which fixes libsmbclient and the test.
Please take a look again.
Thanks,
Andreas
--
Andreas Schneider GPG-ID: CC014E3D
Samba Team asn at samba.org
www.samba.org
-------------- next part --------------
>From 6f0ecb543660b9421d6b923253b7546fd029c874 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 11:44:00 +0200
Subject: [PATCH 1/2] s4:torture: Do not leak memory in libsmbclient test
Found by Coverity.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source4/torture/libsmbclient/libsmbclient.c | 199 ++++++++++++++++++++++------
1 file changed, 159 insertions(+), 40 deletions(-)
diff --git a/source4/torture/libsmbclient/libsmbclient.c b/source4/torture/libsmbclient/libsmbclient.c
index 91579f4b1b7..e762f6e0b60 100644
--- a/source4/torture/libsmbclient/libsmbclient.c
+++ b/source4/torture/libsmbclient/libsmbclient.c
@@ -316,68 +316,187 @@ static bool torture_libsmbclient_readdirplus(struct torture_context *tctx)
return true;
}
-
-/* note the strdup for string options on smbc_set calls. I think libsmbclient is
- * really doing something wrong here: in smbc_free_context libsmbclient just
- * calls free() on the string options so it assumes the callers have malloced
- * them before setting them via smbc_set calls. */
-
-#define TEST_OPTION_INT(option, val) \
- torture_comment(tctx, "Testing smbc_set" #option "\n");\
- smbc_set ##option(ctx, val);\
- torture_comment(tctx, "Testing smbc_get" #option "\n");\
- torture_assert_int_equal(tctx, smbc_get ##option(ctx), val, "failed " #option);
-
-#define TEST_OPTION_STRING(option, val) \
- torture_comment(tctx, "Testing smbc_set" #option "\n");\
- smbc_set ##option(ctx, strdup(val));\
- torture_comment(tctx, "Testing smbc_get" #option "\n");\
- torture_assert_str_equal(tctx, smbc_get ##option(ctx), val, "failed " #option);
-
bool torture_libsmbclient_configuration(struct torture_context *tctx)
{
SMBCCTX *ctx;
+ bool ok = true;
ctx = smbc_new_context();
torture_assert(tctx, ctx, "failed to get new context");
torture_assert(tctx, smbc_init_context(ctx), "failed to init context");
- TEST_OPTION_INT(Debug, DEBUGLEVEL);
- TEST_OPTION_STRING(NetbiosName, "torture_netbios");
- TEST_OPTION_STRING(Workgroup, "torture_workgroup");
- TEST_OPTION_STRING(User, "torture_user");
- TEST_OPTION_INT(Timeout, 12345);
-
+ torture_comment(tctx, "Testing smbc_(set|get)Debug");
+ smbc_setDebug(ctx, DEBUGLEVEL);
+ torture_assert_int_equal_goto(tctx,
+ smbc_getDebug(ctx),
+ DEBUGLEVEL,
+ ok,
+ done,
+ "failed to set DEBUGLEVEL");
+
+ torture_comment(tctx, "Testing smbc_(set|get)NetbiosName");
+ smbc_setNetbiosName(ctx, "torture_netbios");
+ torture_assert_str_equal_goto(tctx,
+ smbc_getNetbiosName(ctx),
+ "torture_netbios",
+ ok,
+ done,
+ "failed to set NetbiosName");
+
+ torture_comment(tctx, "Testing smbc_(set|get)Workgroup");
+ smbc_setWorkgroup(ctx, "torture_workgroup");
+ torture_assert_str_equal_goto(tctx,
+ smbc_getWorkgroup(ctx),
+ "torture_workgroup",
+ ok,
+ done,
+ "failed to set Workgroup");
+
+ torture_comment(tctx, "Testing smbc_(set|get)User");
+ smbc_setUser(ctx, "torture_user");
+ torture_assert_str_equal_goto(tctx,
+ smbc_getUser(ctx),
+ "torture_user",
+ ok,
+ done,
+ "failed to set User");
+
+ torture_comment(tctx, "Testing smbc_(set|get)Timeout");
+ smbc_setTimeout(ctx, 12345);
+ torture_assert_int_equal_goto(tctx,
+ smbc_getTimeout(ctx),
+ 12345,
+ ok,
+ done,
+ "failed to set Timeout");
+
+done:
smbc_free_context(ctx, 1);
- return true;
+ return ok;
}
bool torture_libsmbclient_options(struct torture_context *tctx)
{
SMBCCTX *ctx;
+ bool ok = true;
ctx = smbc_new_context();
torture_assert(tctx, ctx, "failed to get new context");
torture_assert(tctx, smbc_init_context(ctx), "failed to init context");
- TEST_OPTION_INT(OptionDebugToStderr, true);
- TEST_OPTION_INT(OptionFullTimeNames, true);
- TEST_OPTION_INT(OptionOpenShareMode, SMBC_SHAREMODE_DENY_ALL);
- /* FIXME: OptionUserData */
- TEST_OPTION_INT(OptionSmbEncryptionLevel, SMBC_ENCRYPTLEVEL_REQUEST);
- TEST_OPTION_INT(OptionCaseSensitive, false);
- TEST_OPTION_INT(OptionBrowseMaxLmbCount, 2);
- TEST_OPTION_INT(OptionUrlEncodeReaddirEntries, true);
- TEST_OPTION_INT(OptionOneSharePerServer, true);
- TEST_OPTION_INT(OptionUseKerberos, false);
- TEST_OPTION_INT(OptionFallbackAfterKerberos, false);
- TEST_OPTION_INT(OptionNoAutoAnonymousLogin, true);
- TEST_OPTION_INT(OptionUseCCache, true);
-
+ torture_comment(tctx, "Testing smbc_(set|get)OptionDebugToStderr");
+ smbc_setOptionDebugToStderr(ctx, true);
+ torture_assert_goto(tctx,
+ smbc_getOptionDebugToStderr(ctx),
+ ok,
+ done,
+ "failed to set OptionDebugToStderr");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionFullTimeNames");
+ smbc_setOptionFullTimeNames(ctx, true);
+ torture_assert_goto(tctx,
+ smbc_getOptionFullTimeNames(ctx),
+ ok,
+ done,
+ "failed to set OptionFullTimeNames");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionOpenShareMode");
+ smbc_setOptionOpenShareMode(ctx, SMBC_SHAREMODE_DENY_ALL);
+ torture_assert_int_equal_goto(tctx,
+ smbc_getOptionOpenShareMode(ctx),
+ SMBC_SHAREMODE_DENY_ALL,
+ ok,
+ done,
+ "failed to set OptionOpenShareMode");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionUserData");
+ smbc_setOptionUserData(ctx, (void *)discard_const("torture_user_data"));
+ torture_assert_str_equal_goto(tctx,
+ (const char*)smbc_getOptionUserData(ctx),
+ "torture_user_data",
+ ok,
+ done,
+ "failed to set OptionUserData");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionSmbEncryptionLevel");
+ smbc_setOptionSmbEncryptionLevel(ctx, SMBC_ENCRYPTLEVEL_REQUEST);
+ torture_assert_int_equal_goto(tctx,
+ smbc_getOptionSmbEncryptionLevel(ctx),
+ SMBC_ENCRYPTLEVEL_REQUEST,
+ ok,
+ done,
+ "failed to set OptionSmbEncryptionLevel");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionCaseSensitive");
+ smbc_setOptionCaseSensitive(ctx, false);
+ torture_assert_goto(tctx,
+ !smbc_getOptionCaseSensitive(ctx),
+ ok,
+ done,
+ "failed to set OptionCaseSensitive");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionBrowseMaxLmbCount");
+ smbc_setOptionBrowseMaxLmbCount(ctx, 2);
+ torture_assert_int_equal_goto(tctx,
+ smbc_getOptionBrowseMaxLmbCount(ctx),
+ 2,
+ ok,
+ done,
+ "failed to set OptionBrowseMaxLmbCount");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionUrlEncodeReaddirEntries");
+ smbc_setOptionUrlEncodeReaddirEntries(ctx, true);
+ torture_assert_goto(tctx,
+ smbc_getOptionUrlEncodeReaddirEntries(ctx),
+ ok,
+ done,
+ "failed to set OptionUrlEncodeReaddirEntries");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionOneSharePerServer");
+ smbc_setOptionOneSharePerServer(ctx, true);
+ torture_assert_goto(tctx,
+ smbc_getOptionOneSharePerServer(ctx),
+ ok,
+ done,
+ "failed to set OptionOneSharePerServer");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionUseKerberos");
+ smbc_setOptionUseKerberos(ctx, false);
+ torture_assert_goto(tctx,
+ !smbc_getOptionUseKerberos(ctx),
+ ok,
+ done,
+ "failed to set OptionUseKerberos");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionFallbackAfterKerberos");
+ smbc_setOptionFallbackAfterKerberos(ctx, false);
+ torture_assert_goto(tctx,
+ !smbc_getOptionFallbackAfterKerberos(ctx),
+ ok,
+ done,
+ "failed to set OptionFallbackAfterKerberos");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionNoAutoAnonymousLogin");
+ smbc_setOptionNoAutoAnonymousLogin(ctx, true);
+ torture_assert_goto(tctx,
+ smbc_getOptionNoAutoAnonymousLogin(ctx),
+ ok,
+ done,
+ "failed to set OptionNoAutoAnonymousLogin");
+
+ torture_comment(tctx, "Testing smbc_(set|get)OptionUseCCache");
+ smbc_setOptionUseCCache(ctx, true);
+ torture_assert_goto(tctx,
+ smbc_getOptionUseCCache(ctx),
+ ok,
+ done,
+ "failed to set OptionUseCCache");
+
+done:
smbc_free_context(ctx, 1);
- return true;
+ return ok;
}
NTSTATUS torture_libsmbclient_init(TALLOC_CTX *ctx)
--
2.16.3
>From 414859830776ed5689b52005ec39b59dc81069ca Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 16 May 2018 11:46:22 +0200
Subject: [PATCH 2/2] s4:torture: Do not leak file descriptor in smb2 oplock
test
Found by Coverity.
Signed-off-by: Andreas Schneider <asn at samba.org>
---
source4/torture/smb2/oplock.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/source4/torture/smb2/oplock.c b/source4/torture/smb2/oplock.c
index cb1b5edddfe..9561f078c1a 100644
--- a/source4/torture/smb2/oplock.c
+++ b/source4/torture/smb2/oplock.c
@@ -4873,17 +4873,20 @@ static int do_child_process(int pipefd, const char *name)
ret = fcntl(fd, F_SETSIG, RT_SIGNAL_LEASE);
if (ret == -1) {
+ close(fd);
return 3;
}
ret = fcntl(fd, F_SETLEASE, F_WRLCK);
if (ret == -1) {
+ close(fd);
return 4;
}
/* Tell the parent we're ready. */
ret = sys_write(pipefd, &c, 1);
if (ret != 1) {
+ close(fd);
return 5;
}
@@ -4893,14 +4896,17 @@ static int do_child_process(int pipefd, const char *name)
/* Wait for RT_SIGNAL_LEASE or SIGALRM. */
ret = sigsuspend(&empty_set);
if (ret != -1 || errno != EINTR) {
+ close(fd);
return 6;
}
if (got_alarm == 1) {
+ close(fd);
return 10;
}
if (got_break != 1) {
+ close(fd);
return 7;
}
@@ -4913,6 +4919,7 @@ static int do_child_process(int pipefd, const char *name)
/* Remove our lease. */
ret = fcntl(fd, F_SETLEASE, F_UNLCK);
if (ret == -1) {
+ close(fd);
return 8;
}
--
2.16.3
More information about the samba-technical
mailing list