[SSSD] Re: [PATCH] dyndns: Add checks for NULL

2016-07-13 Thread Jakub Hrozek 
On Wed, Jul 13, 2016 at 10:16:22AM +0200, Pavel Březina wrote:
> On 07/12/2016 03:50 PM, Michal Židek wrote:
> > On 07/12/2016 01:36 PM, Michal Židek wrote:
> > > On 07/12/2016 01:15 PM, Pavel Březina wrote:
> > > > On 07/12/2016 12:34 PM, Michal Židek wrote:
> > > > >   state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL);
> > > > 
> > > > LGTM but maybe we should place the check before this line?
> > > 
> > > Not sure... I only added checks for the line with strcmp
> > > (which is where it segfaulted). If I moved
> > > it up where you suggest, there would be the question why
> > > I do not check for example ipa_ctx or ipa_ctx->dyndns_ctx.
> > > 
> > > But if you prefer having it there I can do it, but will
> > > also add a comment that the checks are relevant for the
> > > strcmp line.
> > > 
> > > Michal
> > > 
> > 
> > Ok, I moved it as Pavel suggested and added a comment
> > that should make it clear why the checks are there
> > and also amended the commit message as Jakub suggested.
> > 
> > Michal
> 
> Ack.
> 
> I asked for that since in my opinion it is more natural to set the last
> refresh time only upon successful refresh. But I'm not that familiar with
> dyndns to have strong opinion on what is better, thus it was a quastion.

 * master: b5f61f8963300c9ba011436f234e9e10224aff6d
 * sssd-1-13: ae8c7c4010cb8fda478526771ef12d2bb8bfeffa 

(I hope it's OK I moved the ticket into 1.13.x)
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] dyndns: Add checks for NULL

2016-07-13 Thread Pavel Březina 

On 07/12/2016 03:50 PM, Michal Židek wrote:

On 07/12/2016 01:36 PM, Michal Židek wrote:

On 07/12/2016 01:15 PM, Pavel Březina wrote:

On 07/12/2016 12:34 PM, Michal Židek wrote:

  state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL);


LGTM but maybe we should place the check before this line?


Not sure... I only added checks for the line with strcmp
(which is where it segfaulted). If I moved
it up where you suggest, there would be the question why
I do not check for example ipa_ctx or ipa_ctx->dyndns_ctx.

But if you prefer having it there I can do it, but will
also add a comment that the checks are relevant for the
strcmp line.

Michal



Ok, I moved it as Pavel suggested and added a comment
that should make it clear why the checks are there
and also amended the commit message as Jakub suggested.

Michal


Ack.

I asked for that since in my opinion it is more natural to set the last 
refresh time only upon successful refresh. But I'm not that familiar 
with dyndns to have strong opinion on what is better, thus it was a 
quastion.

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] dyndns: Add checks for NULL

2016-07-12 Thread Michal Židek 

On 07/12/2016 01:36 PM, Michal Židek wrote:

On 07/12/2016 01:15 PM, Pavel Březina wrote:

On 07/12/2016 12:34 PM, Michal Židek wrote:

  state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL);


LGTM but maybe we should place the check before this line?


Not sure... I only added checks for the line with strcmp
(which is where it segfaulted). If I moved
it up where you suggest, there would be the question why
I do not check for example ipa_ctx or ipa_ctx->dyndns_ctx.

But if you prefer having it there I can do it, but will
also add a comment that the checks are relevant for the
strcmp line.

Michal



Ok, I moved it as Pavel suggested and added a comment
that should make it clear why the checks are there
and also amended the commit message as Jakub suggested.

Michal

>From 1339f8b24e9dceb4d71010df831fb177df500b4a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20=C5=BDidek?= 
Date: Tue, 12 Jul 2016 12:11:18 +0200
Subject: [PATCH] dyndns: Add checks for NULL

Fixes:
https://fedorahosted.org/sssd/ticket/3076

We segfaulted in this area once. This patch
makes the code more defensive and adds
some DEBUG messages.

Normally the structures are filled in online
and/or resolve callbacks.
---
 src/providers/ipa/ipa_dyndns.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/src/providers/ipa/ipa_dyndns.c b/src/providers/ipa/ipa_dyndns.c
index 7217c61..dc91077 100644
--- a/src/providers/ipa/ipa_dyndns.c
+++ b/src/providers/ipa/ipa_dyndns.c
@@ -162,6 +162,26 @@ ipa_dyndns_update_send(struct ipa_options *ctx)
 }
 state->ipa_ctx = ctx;
 
+/* The following three checks are here to prevent SEGFAULT
+ * from ticket #3076. */
+if (ctx->service == NULL) {
+DEBUG(SSSDBG_CRIT_FAILURE, "service structure not initialized\n");
+ret = EINVAL;
+goto done;
+}
+
+if (ctx->service->sdap == NULL) {
+DEBUG(SSSDBG_CRIT_FAILURE, "sdap structure not initialized\n");
+ret = EINVAL;
+goto done;
+}
+
+if (ctx->service->sdap->uri == NULL) {
+DEBUG(SSSDBG_CRIT_FAILURE, "LDAP uri not set\n");
+ret = EINVAL;
+goto done;
+}
+
 if (ctx->dyndns_ctx->last_refresh + 60 > time(NULL) ||
 ctx->dyndns_ctx->timer_in_progress) {
 DEBUG(SSSDBG_FUNC_DATA, "Last periodic update ran recently or timer "
-- 
2.5.0

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] dyndns: Add checks for NULL

2016-07-12 Thread Michal Židek 

On 07/12/2016 01:15 PM, Pavel Březina wrote:

On 07/12/2016 12:34 PM, Michal Židek wrote:

  state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL);


LGTM but maybe we should place the check before this line?


Not sure... I only added checks for the line with strcmp
(which is where it segfaulted). If I moved
it up where you suggest, there would be the question why
I do not check for example ipa_ctx or ipa_ctx->dyndns_ctx.

But if you prefer having it there I can do it, but will
also add a comment that the checks are relevant for the
strcmp line.

Michal


___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] dyndns: Add checks for NULL

2016-07-12 Thread Jakub Hrozek 
On Tue, Jul 12, 2016 at 01:15:30PM +0200, Pavel Březina wrote:
> On 07/12/2016 12:34 PM, Michal Židek wrote:
> >   state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL);
> 
> LGTM but maybe we should place the check before this line?

Also in the commit message we should probably say the structures are
normally filled in online callback and/or resolve callback.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] dyndns: Add checks for NULL

2016-07-12 Thread Pavel Březina 

On 07/12/2016 12:34 PM, Michal Židek wrote:

  state->ipa_ctx->dyndns_ctx->last_refresh = time(NULL);


LGTM but maybe we should place the check before this line?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org