[Fwd: Re: [PATCH] Minor cleanup of libnet_LookupName_recv]

Swen Schillig swen at vnet.ibm.com
Tue Feb 13 07:58:25 UTC 2018


Hi Jeremy

Here's the patch set with Christof's reivew comments again.

Please review.

Thanks in advance.

Cheers Swen
-------- Forwarded Message --------
From: Christof Schmitt via samba-technical <samba-technical at lists.samba
.org>
Reply-to: Christof Schmitt <cs at samba.org>
To: Swen Schillig <swen at vnet.ibm.com>
Cc: samba-technical <samba-technical at lists.samba.org>
Subject: Re: [PATCH] Minor cleanup of libnet_LookupName_recv
Date: Thu, 1 Feb 2018 16:25:03 -0700

On Thu, Feb 01, 2018 at 10:11:54PM +0100, Swen Schillig via samba-
technical wrote:

lgtm now.

Reviewed-by: Christof Schmitt <cs at samba.org>

Second reviewer?

Christof

> From e96ab5d00f62e6885dc0f876eb434134efabfdd6 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..a8d41f45335 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 4bfe949af35aa5ec3c8c84e4010edccb186ae52b 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 | 69 ++++++++++++++++++++++++------
> ------------
>  1 file changed, 39 insertions(+), 30 deletions(-)
> 
> diff --git a/source4/libnet/libnet_lookup.c
> b/source4/libnet/libnet_lookup.c
> index a8d41f45335..c4befcacda9 100644
> --- a/source4/libnet/libnet_lookup.c
> +++ b/source4/libnet/libnet_lookup.c
> @@ -381,42 +381,51 @@ NTSTATUS libnet_LookupName_recv(struct
> composite_context *c, TALLOC_CTX *mem_ctx
>  				struct libnet_LookupName *io)
>  {
>  	NTSTATUS status;
> -	struct lookup_name_state *s;
> +	struct lookup_name_state *s = NULL;
> +	struct lsa_RefDomainList *domains = NULL;
> +	struct lsa_TransSidArray *sids = NULL;
>  
>  	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(sta
> tus));
> +		goto done;
> +	}
> +
> +	s = talloc_get_type(c->private_data, struct
> lookup_name_state);
> +
> +	if (*s->lookup.out.count == 0) {
> +		goto success;
> +	}
> +
> +	domains = *s->lookup.out.domains;
> +	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(sta
> tus));
> +		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 92fa8b36557bca3a37debc14a20b97f15f77b7ba 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 c4befcacda9..d43f7ab1bec 100644
> --- a/source4/libnet/libnet_lookup.c
> +++ b/source4/libnet/libnet_lookup.c
> @@ -419,9 +419,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)) {
> +			status = 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)) {
> +			status = NT_STATUS_NO_MEMORY;
> +			goto done;
> +		}
>  	}
>  
>  success:
> -- 
> 2.14.3
> 

-------------- next part --------------
From 73c1127c16a52ad1aae6b3d82c5cc9bb1caa3859 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..a8d41f45335 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 17bba657a908fd842b8dfaede41c27081af4c336 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 | 69 ++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/source4/libnet/libnet_lookup.c b/source4/libnet/libnet_lookup.c
index a8d41f45335..c4befcacda9 100644
--- a/source4/libnet/libnet_lookup.c
+++ b/source4/libnet/libnet_lookup.c
@@ -381,42 +381,51 @@ NTSTATUS libnet_LookupName_recv(struct composite_context *c, TALLOC_CTX *mem_ctx
 				struct libnet_LookupName *io)
 {
 	NTSTATUS status;
-	struct lookup_name_state *s;
+	struct lookup_name_state *s = NULL;
+	struct lsa_RefDomainList *domains = NULL;
+	struct lsa_TransSidArray *sids = NULL;
 
 	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;
+	}
+
+	domains = *s->lookup.out.domains;
+	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 c07bd08cdd239eb700a739c21bfabcc1197fcebe 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 c4befcacda9..d43f7ab1bec 100644
--- a/source4/libnet/libnet_lookup.c
+++ b/source4/libnet/libnet_lookup.c
@@ -419,9 +419,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)) {
+			status = 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)) {
+			status = NT_STATUS_NO_MEMORY;
+			goto done;
+		}
 	}
 
 success:
-- 
2.14.3



More information about the samba-technical mailing list