[SSSD] [sssd][patches] NSS: netgroups minor fixes

Pavel Reichl preichl at redhat.com
Fri Apr 18 12:56:50 UTC 2014


On Fri, 2014-04-18 at 10:32 +0200, Jakub Hrozek wrote:
> On Thu, Apr 17, 2014 at 07:01:35PM +0200, Pavel Reichl wrote:
[snip] 
> > 2nd patch:
> > It is my opinion that function lookup_netgr_step is too long and can be
> > safely (with minimum of changes) divided into 2 separate functions.
> > 
> > 3rd patch:
> > Pavel Brezina asked me to solve memory leak in lookup_netgr_step.
> > 
[snip]
> 
> > From da8ce47f42c6e551432a23ff813c938af6c5721a Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <preichl at redhat.com>
> > Date: Thu, 17 Apr 2014 16:14:11 +0000
> > Subject: [PATCH 3/5] NSS: sysdb_getnetgr refactor
> 
> > +    ret = create_dummy_netgr(step_ctx);
> > +    if (ret != EOK) {
> > +        return ret;
> 
> I think there is a change in behaviour. Previously, if creating the
> negative entry failed, we would have just ignored the failure, the new
> code errors out. I think the failure can be ignored, because at worst,
> there would be a slowdown at the next lookup.

I'm sorry Jakub, but I think you may not be entirely right about this:
following snippet is the only part where function can actually fail but
it's certain copy of original code.

+        if (netgr->name == NULL) {
+            DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed.\n");
+            talloc_free(netgr);
+            return ENOMEM;
+        }

But if you think it's a good idea to ignore even this failure I can
change the code. I just wanted to be 1:1 to original code with this
patch.

> > From 85a1744b803e9d25a82f6ce83a9eaf3d9fdffe16 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <preichl at redhat.com>
> > Date: Thu, 17 Apr 2014 16:52:43 +0000
> > Subject: [PATCH 4/5] NSS: fix memory leak in sysdb_getnetgr
> 
> I'm not opposed to pushing this patch, but in general I think 'leaks'
> like this are not a big issue. The string was allocated on top of a
> per-request context, so when the request finishes, the string would be
> freed together with step_ctx.
> 
> Such growths are much more dangerous in long-running requests like
> nested group resolution, the frontend is usually quite fast and
> efficient.

I agree it's not a big issue, but I've been asked to fix it I and so I
did. 

I would say that main assets of this two patches is improved readability
of this IMO too long and bug prone function. But it is possible that it
is not worth the possibility of introducing new bugs. So feel free to
NACK both patches. No hard feelings.

Pavel Reichl





More information about the sssd-devel mailing list