[PATCH] Minor cleanup of libnet_LookupName_recv
Swen Schillig
swen at vnet.ibm.com
Thu Feb 1 08:46:33 UTC 2018
On Wed, 2018-01-31 at 11:54 -0700, Christof Schmitt wrote:
> On Fri, Jan 26, 2018 at 01:38:14PM +0100, Swen Schillig via samba-
> technical wrote:
> > Hi
> >
> > Another minor cleanup.
> >
> > Please review.
> >
> > Thanks.
> >
> > Cheers Swen.
> > From 04c66afd2fa8a38877ffac0e2774f91e59607003 Mon Sep 17 00:00:00
> > 2001
> > From: Swen Schillig <swen at vnet.ibm.com>
> > Date: Fri, 26 Jan 2018 13:28:58 +0100
> > Subject: [PATCH] Minor cleanup of libnet_LookupName_recv
> >
> > Zero libnet_LookupName out struct before setting results,
> > preventing false result interpretation.
> > Reduce indentation level.
> > Comply with 80 column rule.
> >
> > Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
>
> Would it make sense to split this in two patches? One for the zero
> initialization, and the other one for the logic change to avoid an
> indentation level?
Good point.
Done.
>
> Samba has the ZERO_STRUCT macro, that should be preferred over a
> direct
> call to memset.
Done.
>
> > - io->out.sid =
> > dom_sid_add_rid(mem_ctx, domains->domains[0].sid, io->out.rid);
> > - NT_STATUS_HAVE_NO_MEMORY(i
> > o->out.sid);
> > - io->out.sidstr =
> > dom_sid_string(mem_ctx, io->out.sid);
> > - NT_STATUS_HAVE_NO_MEMORY(i
> > o->out.sidstr);
>
> libcli/util/ntstatus.h
> has a comment that these macros should also be avoided. Maybe that
> could
> become a third patch to replace these with an explicit if() and "goto
> done" in the error case. That would actually fix a memory leak in the
> error path, as currently talloc_free(c) is missing for that case.
Done.
>
> Christof
>
Thanks Christof for your thorough review, really appreciated !
As suggested the patch is split into 3 individual patches.
Please review.
Thanks in advance for your support !.
Cheers Swen.
-------------- next part --------------
From 25467dee127589e58dd1e235e23e00e6767323ec Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at vnet.ibm.com>
Date: Fri, 26 Jan 2018 13:28:58 +0100
Subject: [PATCH 1/3] Zero libnet_LookupName out struct before using
Zero libnet_LookupName out struct before setting results,
preventing false result interpretation.
Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
---
source4/libnet/libnet_lookup.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/source4/libnet/libnet_lookup.c b/source4/libnet/libnet_lookup.c
index bdb99950493..1e17d794782 100644
--- a/source4/libnet/libnet_lookup.c
+++ b/source4/libnet/libnet_lookup.c
@@ -384,14 +384,11 @@ NTSTATUS libnet_LookupName_recv(struct composite_context *c, TALLOC_CTX *mem_ctx
struct lookup_name_state *s;
status = composite_wait(c);
+ ZERO_STRUCT(&io->out);
if (NT_STATUS_IS_OK(status)) {
s = talloc_get_type(c->private_data, struct lookup_name_state);
- io->out.rid = 0;
- io->out.sid = NULL;
- io->out.sidstr = NULL;
-
if (*s->lookup.out.count > 0) {
struct lsa_RefDomainList *domains = *s->lookup.out.domains;
struct lsa_TransSidArray *sids = s->lookup.out.sids;
--
2.14.3
From 58d44833dd1594dc0031853c695615030f19274e Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at vnet.ibm.com>
Date: Thu, 1 Feb 2018 09:02:25 +0100
Subject: [PATCH 2/3] Minor cleanup of libnet_LookupName_recv
Reduce indentation level and comply with 80 column rule.
Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
---
source4/libnet/libnet_lookup.c | 65 +++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 29 deletions(-)
diff --git a/source4/libnet/libnet_lookup.c b/source4/libnet/libnet_lookup.c
index 1e17d794782..14f6a3a6306 100644
--- a/source4/libnet/libnet_lookup.c
+++ b/source4/libnet/libnet_lookup.c
@@ -386,37 +386,44 @@ NTSTATUS libnet_LookupName_recv(struct composite_context *c, TALLOC_CTX *mem_ctx
status = composite_wait(c);
ZERO_STRUCT(&io->out);
- if (NT_STATUS_IS_OK(status)) {
- s = talloc_get_type(c->private_data, struct lookup_name_state);
-
- if (*s->lookup.out.count > 0) {
- struct lsa_RefDomainList *domains = *s->lookup.out.domains;
- struct lsa_TransSidArray *sids = s->lookup.out.sids;
-
- if (domains == NULL || sids == NULL) {
- status = NT_STATUS_UNSUCCESSFUL;
- io->out.error_string = talloc_asprintf(mem_ctx, "Error: %s", nt_errstr(status));
- goto done;
- }
-
- if (sids->count > 0) {
- io->out.rid = sids->sids[0].rid;
- io->out.sid_type = sids->sids[0].sid_type;
- if (domains->count > 0) {
- io->out.sid = dom_sid_add_rid(mem_ctx, domains->domains[0].sid, io->out.rid);
- NT_STATUS_HAVE_NO_MEMORY(io->out.sid);
- io->out.sidstr = dom_sid_string(mem_ctx, io->out.sid);
- NT_STATUS_HAVE_NO_MEMORY(io->out.sidstr);
- }
- }
- }
-
- io->out.error_string = talloc_strdup(mem_ctx, "Success");
-
- } else if (!NT_STATUS_IS_OK(status)) {
- io->out.error_string = talloc_asprintf(mem_ctx, "Error: %s", nt_errstr(status));
+ if (!NT_STATUS_IS_OK(status)) {
+ io->out.error_string = talloc_asprintf(mem_ctx, "Error: %s",
+ nt_errstr(status));
+ goto done;
+ }
+
+ s = talloc_get_type(c->private_data, struct lookup_name_state);
+
+ if (*s->lookup.out.count == 0) {
+ goto success;
+ }
+
+ struct lsa_RefDomainList *domains = *s->lookup.out.domains;
+ struct lsa_TransSidArray *sids = s->lookup.out.sids;
+
+ if (domains == NULL || sids == NULL) {
+ status = NT_STATUS_UNSUCCESSFUL;
+ io->out.error_string = talloc_asprintf(mem_ctx, "Error: %s",
+ nt_errstr(status));
+ goto done;
+ }
+
+ if (sids->count == 0) {
+ goto success;
+ }
+
+ io->out.rid = sids->sids[0].rid;
+ io->out.sid_type = sids->sids[0].sid_type;
+ if (domains->count > 0) {
+ io->out.sid = dom_sid_add_rid(mem_ctx, domains->domains[0].sid,
+ io->out.rid);
+ NT_STATUS_HAVE_NO_MEMORY(io->out.sid);
+ io->out.sidstr = dom_sid_string(mem_ctx, io->out.sid);
+ NT_STATUS_HAVE_NO_MEMORY(io->out.sidstr);
}
+success:
+ io->out.error_string = talloc_strdup(mem_ctx, "Success");
done:
talloc_free(c);
return status;
--
2.14.3
From d41273d7dc45eb86583fe049cca1efe5e5566945 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at vnet.ibm.com>
Date: Thu, 1 Feb 2018 09:39:02 +0100
Subject: [PATCH 3/3] Replace NT_STATUS_HAVE_NO_MEMORY macro
Replaced NT_STATUS_HAVE_NO_MEMORY macro and fixed
memory leakin error-path.
Signed-off-by: Swen Schillig <swen at vnet.ibm.com>
---
source4/libnet/libnet_lookup.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/source4/libnet/libnet_lookup.c b/source4/libnet/libnet_lookup.c
index 14f6a3a6306..bf58bac8748 100644
--- a/source4/libnet/libnet_lookup.c
+++ b/source4/libnet/libnet_lookup.c
@@ -417,9 +417,15 @@ NTSTATUS libnet_LookupName_recv(struct composite_context *c, TALLOC_CTX *mem_ctx
if (domains->count > 0) {
io->out.sid = dom_sid_add_rid(mem_ctx, domains->domains[0].sid,
io->out.rid);
- NT_STATUS_HAVE_NO_MEMORY(io->out.sid);
+ if (unlikely(io->out.sid == NULL)) {
+ statust = NT_STATUS_NO_MEMORY;
+ goto done;
+ }
io->out.sidstr = dom_sid_string(mem_ctx, io->out.sid);
- NT_STATUS_HAVE_NO_MEMORY(io->out.sidstr);
+ if (unlikely(io->out.sidstr == NULL)) {
+ statust = NT_STATUS_NO_MEMORY;
+ goto done;
+ }
}
success:
--
2.14.3
More information about the samba-technical
mailing list