On Thu, Jul 07, 2016 at 12:39:28PM +0200, Pavel Březina wrote:
> On 07/07/2016 12:34 PM, Jakub Hrozek wrote:
> > On Thu, Jul 07, 2016 at 10:16:03AM +0200, Sumit Bose wrote:
> > > resend
> > > ----- Forwarded message from Sumit Bose <sb...@redhat.com> -----
> > >
> > > Date: Wed, 6 Jul 2016 11:13:48 +0200
> > > From: Sumit Bose <sb...@redhat.com>
> > > To: sssd-devel@lists.fedorahosted.org
> > > Subject: Re: [SSSD] [PATCH] LDAP: Lookup services by all protocols unless
> > > a
> > > protocol is specified
> > > Message-ID:
> > > <20160706091348.GD29143@p.Speedport_W_724V_Typ_A_05011603_00_009>
> > > References: <20160705103025.GB24232@hendrix>
> > > MIME-Version: 1.0
> > > Content-Type: text/plain; charset=us-ascii
> > > Content-Disposition: inline
> > > In-Reply-To: <20160705103025.GB24232@hendrix>
> > > User-Agent: Mutt/1.6.1 (2016-04-27)
> > >
> > > On Tue, Jul 05, 2016 at 12:30:25PM +0200, Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > the attached patch makes service lookups great again.
> > > >
> > > > To reproduce, just run:
> > > > getent service -s sss ldap
> > > > before the patch we would look up ipService="" because DP gives us an
> > > > empty string after the recent DP patches.
> > >
> > > > From ba9834637b3cc0d7d98f704ba70f9dcb6f9a70e9 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek <jhro...@redhat.com>
> > > > Date: Tue, 5 Jul 2016 12:23:23 +0200
> > > > Subject: [PATCH] LDAP: Lookup services by all protocols unless a
> > > > protocol is
> > > > specified
> > > >
> > > > The DP refactoring changed the way we handle strings from sbus. We no
> > > > longer receive NULL strings, but empty strings instead.
> > > > ---
> > > > src/providers/ldap/ldap_id_services.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/providers/ldap/ldap_id_services.c
> > > > b/src/providers/ldap/ldap_id_services.c
> > > > index
> > > > 77215127b53297d840eaa4d2f35a75eedb085e43..db47e3fc55eea969371d61c3c5ac7f818196f3d5
> > > > 100644
> > > > --- a/src/providers/ldap/ldap_id_services.c
> > > > +++ b/src/providers/ldap/ldap_id_services.c
> > > > @@ -114,7 +114,7 @@ services_get_send(TALLOC_CTX *mem_ctx,
> > > > ret = sss_filter_sanitize(state, name, &clean_name);
> > > > if (ret != EOK) goto error;
> > > >
> > > > - if (protocol) {
> > > > + if (protocol && protocol[0] != '\0') {
> > > > ret = sss_filter_sanitize(state, protocol, &clean_protocol);
> > > > if (ret != EOK) goto error;
> > > > }
> > >
> > > since the sysdb calls later on in the request expect protocol==NULL as
> > > well I would suggest something like
> > >
> > > state->sysdb = sdom->dom->sysdb;
> > > state->name = name;
> > > state->protocol = protocol;
> > > + if (state->protocol[0] == '\0') {
> > > + state->protocol = NULL;
> > > + }
> > > state->filter_type = filter_type;
> > > state->noexist_delete = noexist_delete;
> > >
> > >
> > > in only use state->protocol in the request.
> >
> > OK, done (we use state->protocol up to a point, then clean_protocol). I
> > also added a check to cover the unlikely situation where sbus might give
> > us a NULL string, just to error and don't crash.
>
> Hi, nice catch. I think it may be better to amend check_and_parse_filter()
> in dp_target_id.c so it assign NULL instead of "" in data->filter_value (and
> other) using SBUS_SET_STRING(). Though I didn't check if other look up types
> (user/group/...) don't expect empty string instead.
Yes, this makes sense and a quick git grep shows that the providers
expect NULL, not empty string (for example for enumeration which we
already special-case in check_and_parse_filter). But at this point,
I would prefer to push this patch, release 1.14.0 and then patch
check_and_parse_filter() and run tests again to make sure we don't break
some strange corner case with a late sbus fix.
Attached is a new patch that doesn't error out if protocol is NULL.
>From aafc94e76722019f8b921213a48867fb97be6a03 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Tue, 5 Jul 2016 12:23:23 +0200
Subject: [PATCH] LDAP: Lookup services by all protocols unless a protocol is
specified
The DP refactoring changed the way we handle strings from sbus. We no
longer receive NULL strings, but empty strings instead.
---
src/providers/ldap/ldap_id_services.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/providers/ldap/ldap_id_services.c
b/src/providers/ldap/ldap_id_services.c
index
77215127b53297d840eaa4d2f35a75eedb085e43..401228c20af31ae2df9bb3d35ed25fb6f06b1839
100644
--- a/src/providers/ldap/ldap_id_services.c
+++ b/src/providers/ldap/ldap_id_services.c
@@ -89,6 +89,9 @@ services_get_send(TALLOC_CTX *mem_ctx,
state->sysdb = sdom->dom->sysdb;
state->name = name;
state->protocol = protocol;
+ if (state->protocol != NULL && state->protocol[0] == '\0') {
+ state->protocol = NULL;
+ }
state->filter_type = filter_type;
state->noexist_delete = noexist_delete;
@@ -114,8 +117,8 @@ services_get_send(TALLOC_CTX *mem_ctx,
ret = sss_filter_sanitize(state, name, &clean_name);
if (ret != EOK) goto error;
- if (protocol) {
- ret = sss_filter_sanitize(state, protocol, &clean_protocol);
+ if (state->protocol != NULL) {
+ ret = sss_filter_sanitize(state, state->protocol, &clean_protocol);
if (ret != EOK) goto error;
}
--
2.4.11
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
