[PATCH] Use a stackframe for memory management in _wbint_QueryGroupList

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Use a stackframe for memory management in _wbint_QueryGroupList

Samba - samba-technical mailing list
Hi,

I'm fixing that function right now for 3.6 and saw that some TALLOC_FREE are
missing. For master I decided to use a stackframe to make it simpler to clean
up.

Please review and push if OK.


Thanks,


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

0001-s3-winbindd-Use-a-stackframe-for-memory-management.patch1.txt (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use a stackframe for memory management in _wbint_QueryGroupList

Samba - samba-technical mailing list
On Thu, Dec 07, 2017 at 08:29:03AM +0100, Andreas Schneider via samba-technical wrote:
> Hi,
>
> I'm fixing that function right now for 3.6 and saw that some TALLOC_FREE are
> missing. For master I decided to use a stackframe to make it simpler to clean
> up.
>
> Please review and push if OK.

Is the talloc_steal() really needed ?

We already have:

  result = talloc_array(r->out.groups, struct wbint_Principal,
       num_total);

(NB. result is allocated off r->out.groups) so doing:

 + r->out.groups->principals = talloc_steal(r->out.groups, result);

is at best redundant. How does talloc_steal() react when
the target is the same as the parent ?

Also, I prefer talloc_move() (yeah I know, I invented it) as it
leaves the old pointer as NULL for safety :-).

Also, if you're fixing any real memory leaks here we probably need
a bug report for back-ports.

Cheers,

        Jeremy.

>
>
> Andreas
>
> --
> Andreas Schneider                   GPG-ID: CC014E3D
> Samba Team                             [hidden email]
> www.samba.org

> From 1a064ead31de305e9c607e6231260ae6a4d194d6 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <[hidden email]>
> Date: Wed, 6 Dec 2017 18:48:47 +0100
> Subject: [PATCH] s3:winbindd: Use a stackframe for memory management in
>  _wbint_QueryGroupList
>
> Signed-off-by: Andreas Schneider <[hidden email]>
> ---
>  source3/winbindd/winbindd_dual_srv.c | 40 +++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/source3/winbindd/winbindd_dual_srv.c b/source3/winbindd/winbindd_dual_srv.c
> index 797a9d95493..27175ccf159 100644
> --- a/source3/winbindd/winbindd_dual_srv.c
> +++ b/source3/winbindd/winbindd_dual_srv.c
> @@ -380,6 +380,7 @@ NTSTATUS _wbint_LookupGroupMembers(struct pipes_struct *p,
>  NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>         struct wbint_QueryGroupList *r)
>  {
> + TALLOC_CTX *frame = NULL;
>   struct winbindd_domain *domain = wb_child_domain();
>   uint32_t i;
>   uint32_t num_local_groups = 0;
> @@ -389,13 +390,15 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>   uint32_t ti = 0;
>   uint64_t num_total = 0;
>   struct wbint_Principal *result;
> - NTSTATUS status;
> + NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
>   bool include_local_groups = false;
>  
>   if (domain == NULL) {
>   return NT_STATUS_REQUEST_NOT_ACCEPTED;
>   }
>  
> + frame = talloc_stackframe();
> +
>   switch (lp_server_role()) {
>   case ROLE_ACTIVE_DIRECTORY_DC:
>   if (domain->internal) {
> @@ -416,32 +419,34 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>   }
>  
>   if (include_local_groups) {
> - status = wb_cache_enum_local_groups(domain, talloc_tos(),
> + status = wb_cache_enum_local_groups(domain, frame,
>      &num_local_groups,
>      &local_groups);
>   reset_cm_connection_on_error(domain, status);
>   if (!NT_STATUS_IS_OK(status)) {
> - return status;
> + goto out;
>   }
>   }
>  
> - status = wb_cache_enum_dom_groups(domain, talloc_tos(),
> + status = wb_cache_enum_dom_groups(domain, frame,
>    &num_dom_groups,
>    &dom_groups);
>   reset_cm_connection_on_error(domain, status);
>   if (!NT_STATUS_IS_OK(status)) {
> - return status;
> + goto out;
>   }
>  
>   num_total = num_local_groups + num_dom_groups;
>   if (num_total > UINT32_MAX) {
> - return NT_STATUS_INTERNAL_ERROR;
> + status = NT_STATUS_INTERNAL_ERROR;
> + goto out;
>   }
>  
>   result = talloc_array(r->out.groups, struct wbint_Principal,
>        num_total);
>   if (result == NULL) {
> - return NT_STATUS_NO_MEMORY;
> + status = NT_STATUS_NO_MEMORY;
> + goto out;
>   }
>  
>   for (i = 0; i < num_local_groups; i++) {
> @@ -452,14 +457,11 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>   rg->type = SID_NAME_ALIAS;
>   rg->name = talloc_strdup(result, lg->acct_name);
>   if (rg->name == NULL) {
> - TALLOC_FREE(result);
> - TALLOC_FREE(dom_groups);
> - TALLOC_FREE(local_groups);
> - return NT_STATUS_NO_MEMORY;
> + status = NT_STATUS_NO_MEMORY;
> + goto out;
>   }
>   }
>   num_local_groups = 0;
> - TALLOC_FREE(local_groups);
>  
>   for (i = 0; i < num_dom_groups; i++) {
>   struct wb_acct_info *dg = &dom_groups[i];
> @@ -469,19 +471,19 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>   rg->type = SID_NAME_DOM_GRP;
>   rg->name = talloc_strdup(result, dg->acct_name);
>   if (rg->name == NULL) {
> - TALLOC_FREE(result);
> - TALLOC_FREE(dom_groups);
> - TALLOC_FREE(local_groups);
> - return NT_STATUS_NO_MEMORY;
> + status = NT_STATUS_NO_MEMORY;
> + goto out;
>   }
>   }
>   num_dom_groups = 0;
> - TALLOC_FREE(dom_groups);
>  
>   r->out.groups->num_principals = ti;
> - r->out.groups->principals = result;
> + r->out.groups->principals = talloc_steal(r->out.groups, result);
>  
> - return NT_STATUS_OK;
> + status = NT_STATUS_OK;
> +out:
> + TALLOC_FREE(frame);
> + return status;
>  }
>  
>  NTSTATUS _wbint_QueryUserRidList(struct pipes_struct *p,
> --
> 2.15.1
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use a stackframe for memory management in _wbint_QueryGroupList

Samba - samba-technical mailing list
On Thursday, 7 December 2017 18:57:34 CET Jeremy Allison wrote:
> On Thu, Dec 07, 2017 at 08:29:03AM +0100, Andreas Schneider via samba-
technical wrote:
> > Hi,
> >
> > I'm fixing that function right now for 3.6 and saw that some TALLOC_FREE
> > are missing. For master I decided to use a stackframe to make it simpler
> > to clean up.
> >
> > Please review and push if OK.
>
> Is the talloc_steal() really needed ?

Ups, during fixing that I forgot to change this back to frame. So
talloc_array() should allocate on the frame that the array is freed on error
and talloc_move should move it in the end ...

> We already have:
>
>   result = talloc_array(r->out.groups, struct wbint_Principal,
>        num_total);
>
> (NB. result is allocated off r->out.groups) so doing:
>
>  + r->out.groups->principals = talloc_steal(r->out.groups, result);
>
> is at best redundant. How does talloc_steal() react when
> the target is the same as the parent ?
>
> Also, I prefer talloc_move() (yeah I know, I invented it) as it
> leaves the old pointer as NULL for safety :-).
Ok, I used talloc_move() now.
 
> Also, if you're fixing any real memory leaks here we probably need
> a bug report for back-ports.

Not really, we allocate on talloc_tos() it probably doesn't live long enough,
but some error path directly free it and others don't. This just makes sure
every error path directly cleans up. I don't think we need a bug.

Updated patch attached.


Thanks,


        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

0001-s3-winbindd-Use-a-stackframe-for-memory-management.patch2.txt (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use a stackframe for memory management in _wbint_QueryGroupList

Samba - samba-technical mailing list
On Thu, Dec 07, 2017 at 07:04:42PM +0100, Andreas Schneider wrote:

> On Thursday, 7 December 2017 18:57:34 CET Jeremy Allison wrote:
> > On Thu, Dec 07, 2017 at 08:29:03AM +0100, Andreas Schneider via samba-
> technical wrote:
> > > Hi,
> > >
> > > I'm fixing that function right now for 3.6 and saw that some TALLOC_FREE
> > > are missing. For master I decided to use a stackframe to make it simpler
> > > to clean up.
> > >
> > > Please review and push if OK.
> >
> > Is the talloc_steal() really needed ?
>
> Ups, during fixing that I forgot to change this back to frame. So
> talloc_array() should allocate on the frame that the array is freed on error
> and talloc_move should move it in the end ...
>
> > We already have:
> >
> >   result = talloc_array(r->out.groups, struct wbint_Principal,
> >        num_total);
> >
> > (NB. result is allocated off r->out.groups) so doing:
> >
> >  + r->out.groups->principals = talloc_steal(r->out.groups, result);
> >
> > is at best redundant. How does talloc_steal() react when
> > the target is the same as the parent ?
> >
> > Also, I prefer talloc_move() (yeah I know, I invented it) as it
> > leaves the old pointer as NULL for safety :-).
>
> Ok, I used talloc_move() now.
>  
> > Also, if you're fixing any real memory leaks here we probably need
> > a bug report for back-ports.
>
> Not really, we allocate on talloc_tos() it probably doesn't live long enough,
> but some error path directly free it and others don't. This just makes sure
> every error path directly cleans up. I don't think we need a bug.
>
> Updated patch attached.

LGTM. Pushed !





> --
> Andreas Schneider                   GPG-ID: CC014E3D
> Samba Team                             [hidden email]
> www.samba.org

> From d6bbf9ff01ff7a57935264e7676ce177f5937223 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <[hidden email]>
> Date: Wed, 6 Dec 2017 18:48:47 +0100
> Subject: [PATCH] s3:winbindd: Use a stackframe for memory management in
>  _wbint_QueryGroupList
>
> Signed-off-by: Andreas Schneider <[hidden email]>
> ---
>  source3/winbindd/winbindd_dual_srv.c | 43 ++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/source3/winbindd/winbindd_dual_srv.c b/source3/winbindd/winbindd_dual_srv.c
> index 797a9d95493..81fcf855847 100644
> --- a/source3/winbindd/winbindd_dual_srv.c
> +++ b/source3/winbindd/winbindd_dual_srv.c
> @@ -380,6 +380,7 @@ NTSTATUS _wbint_LookupGroupMembers(struct pipes_struct *p,
>  NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>         struct wbint_QueryGroupList *r)
>  {
> + TALLOC_CTX *frame = NULL;
>   struct winbindd_domain *domain = wb_child_domain();
>   uint32_t i;
>   uint32_t num_local_groups = 0;
> @@ -389,13 +390,15 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>   uint32_t ti = 0;
>   uint64_t num_total = 0;
>   struct wbint_Principal *result;
> - NTSTATUS status;
> + NTSTATUS status = NT_STATUS_UNSUCCESSFUL;
>   bool include_local_groups = false;
>  
>   if (domain == NULL) {
>   return NT_STATUS_REQUEST_NOT_ACCEPTED;
>   }
>  
> + frame = talloc_stackframe();
> +
>   switch (lp_server_role()) {
>   case ROLE_ACTIVE_DIRECTORY_DC:
>   if (domain->internal) {
> @@ -416,32 +419,33 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>   }
>  
>   if (include_local_groups) {
> - status = wb_cache_enum_local_groups(domain, talloc_tos(),
> + status = wb_cache_enum_local_groups(domain, frame,
>      &num_local_groups,
>      &local_groups);
>   reset_cm_connection_on_error(domain, status);
>   if (!NT_STATUS_IS_OK(status)) {
> - return status;
> + goto out;
>   }
>   }
>  
> - status = wb_cache_enum_dom_groups(domain, talloc_tos(),
> + status = wb_cache_enum_dom_groups(domain, frame,
>    &num_dom_groups,
>    &dom_groups);
>   reset_cm_connection_on_error(domain, status);
>   if (!NT_STATUS_IS_OK(status)) {
> - return status;
> + goto out;
>   }
>  
>   num_total = num_local_groups + num_dom_groups;
>   if (num_total > UINT32_MAX) {
> - return NT_STATUS_INTERNAL_ERROR;
> + status = NT_STATUS_INTERNAL_ERROR;
> + goto out;
>   }
>  
> - result = talloc_array(r->out.groups, struct wbint_Principal,
> -      num_total);
> + result = talloc_array(frame, struct wbint_Principal, num_total);
>   if (result == NULL) {
> - return NT_STATUS_NO_MEMORY;
> + status = NT_STATUS_NO_MEMORY;
> + goto out;
>   }
>  
>   for (i = 0; i < num_local_groups; i++) {
> @@ -452,14 +456,11 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>   rg->type = SID_NAME_ALIAS;
>   rg->name = talloc_strdup(result, lg->acct_name);
>   if (rg->name == NULL) {
> - TALLOC_FREE(result);
> - TALLOC_FREE(dom_groups);
> - TALLOC_FREE(local_groups);
> - return NT_STATUS_NO_MEMORY;
> + status = NT_STATUS_NO_MEMORY;
> + goto out;
>   }
>   }
>   num_local_groups = 0;
> - TALLOC_FREE(local_groups);
>  
>   for (i = 0; i < num_dom_groups; i++) {
>   struct wb_acct_info *dg = &dom_groups[i];
> @@ -469,19 +470,19 @@ NTSTATUS _wbint_QueryGroupList(struct pipes_struct *p,
>   rg->type = SID_NAME_DOM_GRP;
>   rg->name = talloc_strdup(result, dg->acct_name);
>   if (rg->name == NULL) {
> - TALLOC_FREE(result);
> - TALLOC_FREE(dom_groups);
> - TALLOC_FREE(local_groups);
> - return NT_STATUS_NO_MEMORY;
> + status = NT_STATUS_NO_MEMORY;
> + goto out;
>   }
>   }
>   num_dom_groups = 0;
> - TALLOC_FREE(dom_groups);
>  
>   r->out.groups->num_principals = ti;
> - r->out.groups->principals = result;
> + r->out.groups->principals = talloc_move(r->out.groups, &result);
>  
> - return NT_STATUS_OK;
> + status = NT_STATUS_OK;
> +out:
> + TALLOC_FREE(frame);
> + return status;
>  }
>  
>  NTSTATUS _wbint_QueryUserRidList(struct pipes_struct *p,
> --
> 2.15.1
>