[PATCH] IPA: Handle NULL members in process_members()

[Jakub Hrozek]

Hi,

I stumbled upon a crash when reviewing Pavel's recent subdomain fix against
an IPA 3.3 server. I'm not sure if it's the right way to fix the problem,
normally I would prefer a { NULL } array so the loop falls through, but
this quick patch allowed me to at least continue with the review..

[Lukas Slebodnik]

On (14/10/14 14:27), Jakub Hrozek wrote:
> Hi,
>
> I stumbled upon a crash when reviewing Pavel's recent subdomain fix against
> an IPA 3.3 server. I'm not sure if it's the right way to fix the problem,
> normally I would prefer a { NULL } array so the loop falls through, but
> this quick patch allowed me to at least continue with the review..
> From 4eb6b369127a4ef896036cdcbef7fd03f5b10e7f Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Tue, 14 Oct 2014 14:15:25 +0200
> Subject: [PATCH] IPA: Handle NULL members in process_members()
>
> src/providers/ipa/ipa_s2n_exop.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c
> index 0d5c35678efda5a87fa0d9164542cbb89ff4edf9..680340688fd983bd131d382b11cad2963a8088b2 100644
> --- a/src/providers/ipa/ipa_s2n_exop.c
> +++ b/src/providers/ipa/ipa_s2n_exop.c
> @@ -1126,6 +1126,11 @@ static errno_t process_members(struct sss_domain_info *domain,
>     struct sss_domain_info *obj_domain;
>     struct sss_domain_info *parent_domain;
> if (members == NULL) {
>    DEBUG(SSSDBG_TRACE_INTERNAL, "No members\n");
>    return EOK;
> }
> tmp_ctx = talloc_new(NULL);
>   if (tmp_ctx == NULL) {
>       DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");

make sense.

It would be good to log name of group, but it cannot be done
in function process_members.

There are possible solutions:
   * add extra argument just for debug message
   * return different error code, which will be special cased in calling
     function (ipa_s2n_save_objects).
   * add another trace debug message before calling function process_members.

LS

[Jakub Hrozek]

On Tue, Oct 14, 2014 at 05:28:36PM +0200, Lukas Slebodnik wrote:
> On (14/10/14 14:27), Jakub Hrozek wrote:
> > Hi,
> >
> > I stumbled upon a crash when reviewing Pavel's recent subdomain fix against
> > an IPA 3.3 server. I'm not sure if it's the right way to fix the problem,
> > normally I would prefer a { NULL } array so the loop falls through, but
> > this quick patch allowed me to at least continue with the review..
> > From 4eb6b369127a4ef896036cdcbef7fd03f5b10e7f Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Tue, 14 Oct 2014 14:15:25 +0200
> > Subject: [PATCH] IPA: Handle NULL members in process_members()
> >
> > src/providers/ipa/ipa_s2n_exop.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c
> > index 0d5c35678efda5a87fa0d9164542cbb89ff4edf9..680340688fd983bd131d382b11cad2963a8088b2 100644
> > --- a/src/providers/ipa/ipa_s2n_exop.c
> > +++ b/src/providers/ipa/ipa_s2n_exop.c
> > @@ -1126,6 +1126,11 @@ static errno_t process_members(struct sss_domain_info *domain,
> >     struct sss_domain_info *obj_domain;
> >     struct sss_domain_info *parent_domain;
> > if (members == NULL) {
> >    DEBUG(SSSDBG_TRACE_INTERNAL, "No members\n");
> >    return EOK;
> > }
> > tmp_ctx = talloc_new(NULL);
> >   if (tmp_ctx == NULL) {
> >       DEBUG(SSSDBG_OP_FAILURE, "talloc_new failed.\n");
>
> make sense.
>
> It would be good to log name of group, but it cannot be done
> in function process_members.
>
> There are possible solutions:
> add extra argument just for debug message
> return different error code, which will be special cased in calling
> function (ipa_s2n_save_objects).
> add another trace debug message before calling function process_members.
>
> LS

Was that an ack?

[Jakub Hrozek]

On Tue, Oct 14, 2014 at 05:28:36PM +0200, Lukas Slebodnik wrote:
> add another trace debug message before calling function process_members.

I went with this last option.

[Lukas Slebodnik]

On (20/10/14 22:31), Jakub Hrozek wrote:
> On Tue, Oct 14, 2014 at 05:28:36PM +0200, Lukas Slebodnik wrote:
> > add another trace debug message before calling function process_members.
>
> I went with this last option.

Thank you.
> From 511c3f922f4c36302bed5270f019dc336b5e82dc Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Tue, 14 Oct 2014 14:15:25 +0200
> Subject: [PATCH] IPA: Handle NULL members in process_members()
>
> src/providers/ipa/ipa_s2n_exop.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c

ACK

LS

[Jakub Hrozek]

On Tue, Oct 21, 2014 at 12:40:54AM +0200, Lukas Slebodnik wrote:
> On (20/10/14 22:31), Jakub Hrozek wrote:
> > On Tue, Oct 14, 2014 at 05:28:36PM +0200, Lukas Slebodnik wrote:
> > > add another trace debug message before calling function process_members.
> >
> > I went with this last option.
>
> Thank you.
> > From 511c3f922f4c36302bed5270f019dc336b5e82dc Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Tue, 14 Oct 2014 14:15:25 +0200
> > Subject: [PATCH] IPA: Handle NULL members in process_members()
> >
> > src/providers/ipa/ipa_s2n_exop.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/src/providers/ipa/ipa_s2n_exop.c b/src/providers/ipa/ipa_s2n_exop.c
>
> ACK
>
> LS

* master: 35863245aa8b8404caedfc95e68b3aa5af14542e