[SSSD] [Patch] Added null checks to authtok module

Pallavi Jha pallavikumarijha at gmail.com
Sun Dec 8 07:45:45 UTC 2013


Checks from static function are removed. Kindly review it.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 8 December 2013 03:25, Lukas Slebodnik <lslebodn at redhat.com> wrote:

> On (08/12/13 02:05), Pallavi Jha wrote:
> >Hi,
> >
> >Requested changes have been done. Kindly review the patch.
> >
> >With regards
> >Pallavi Kumari Jha
> >Computer Science Engineering
> >
> >
> >On 22 November 2013 20:34, Jakub Hrozek <jhrozek at redhat.com> wrote:
> >
> >> On Fri, Nov 22, 2013 at 09:41:16AM -0500, Simo Sorce wrote:
> >> > On Fri, 2013-11-22 at 18:42 +0530, Pallavi Jha wrote:
> >> > >
> >> > > Hello,
> >> > >
> >> > > Patch with required changes is attached with the mail. Please review
> >> > > it.
> >> > >
> >> > Thank you, comments inline.
> >> >
> >> > > From bfcbe7d16e325dffb807da096a27955c579174e0 Mon Sep 17 00:00:00
> 2001
> >> > > From: Pallavi Jha <pallavikumarijha at gmail.com>
> >> > > Date: Sat, 16 Nov 2013 16:23:01 +0530
> >> > > Subject: [PATCH] added null checks to authtok module
> >> > >
> >> > > ---
> >> > >  src/util/authtok.c | 30 ++++++++++++++++++++++++++++--
> >> > >  1 file changed, 28 insertions(+), 2 deletions(-)
> >> > >
> >> > > diff --git a/src/util/authtok.c b/src/util/authtok.c
> >> > > index
> >> > >
> >>
> 83e6a1c942a0abc136422bf062be47cbc687bb72..2c67c038bfbeebade462b15eef72e7827fb9f7e4
> >> 100644
> >> > > --- a/src/util/authtok.c
> >> > > +++ b/src/util/authtok.c
> >> > > @@ -27,11 +27,17 @@ struct sss_auth_token {
> >> > >
> >> > >  enum sss_authtok_type sss_authtok_get_type(struct sss_auth_token
> >> > > *tok)
> >> > >  {
> >> > > +    if (!tok) {
> >> > > +        return -1;
> >> >
> >> > -1 is not a valid sss_authtok_type, maybe we should just return
> >> > SSS_AUTHTOK_TYPE_EMPTY, or you need to add -1 to the enumeration with
> an
> >> > appropriate name SSS_AUTHTOK_TYPE_FAULTY ?
> >> >
> >> > I wonder also if we should just let also some public functions fail,
> >> > this should simply never happen ...
> >>
> >> Thanks Simo, we were discussing the same with Lukas right now and were
> >> not sure either :-)
> >>
> >> I'm in favor or not checking this particular function.
> >> SSS_AUTHTOK_TYPE_EMPTY is a valid type, while NULL authtok is simply
> >> invalid. I would rather add SSS_AUTHTOK_TYPE_INVALID or similar, but
> >> adding an enum type just for a check (and then having to handle it in
> >> all the switch-case statements in all consumers) seems too much.
> >>
> >> So I'm in favor of removing this check and only checking the other
> >> functions.
> >>
> >> >
> >> > > +    }
> >> > >      return tok->type;
> >> > >  }
> >> > >
> >> > >  size_t sss_authtok_get_size(struct sss_auth_token *tok)
> >> > >  {
> >> > > +    if (!tok) {
> >> > > +        return 0;
> >> > > +    }
> >> > >      switch (tok->type) {
> >> > >      case SSS_AUTHTOK_TYPE_PASSWORD:
> >> > >      case SSS_AUTHTOK_TYPE_CCFILE:
> >> > > @@ -45,12 +51,18 @@ size_t sss_authtok_get_size(struct
> sss_auth_token
> >> > > *tok)
> >> > >
> >> > >  uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
> >> > >  {
> >> > > +    if (!tok) {
> >> > > +        return NULL;
> >> > > +    }
> >> > >      return tok->data;
> >> > >  }
> >> > >
> >> > >  errno_t sss_authtok_get_password(struct sss_auth_token *tok,
> >> > >                                   const char **pwd, size_t *len)
> >> > >  {
> >> > > +    if (!tok) {
> >> > > +        return EINVAL;
> >> > > +    }
> >> >
> >> > Perhaps EFAULT (as in bad address) is a better error message here ?
> >> > Not 100% sure.
> >> >
> >> > >      switch (tok->type) {
> >> > >      case SSS_AUTHTOK_TYPE_EMPTY:
> >> > >          return ENOENT;
> >> > > @@ -70,6 +82,9 @@ errno_t sss_authtok_get_password(struct
> >> > > sss_auth_token *tok,
> >> > >  errno_t sss_authtok_get_ccfile(struct sss_auth_token *tok,
> >> > >                                 const char **ccfile, size_t *len)
> >> > >  {
> >> > > +    if (!tok) {
> >> > > +        return EINVAL;
> >> > > +    }
> >> > >      switch (tok->type) {
> >> > >      case SSS_AUTHTOK_TYPE_EMPTY:
> >> > >          return ENOENT;
> >> > > @@ -91,11 +106,16 @@ static errno_t sss_authtok_set_string(struct
> >> > > sss_auth_token *tok,
> >> > >                                        const char *context_name,
> >> > >                                        const char *str, size_t len)
> >> > >  {
> >> > > +    if (!tok || !str) {
> >> > > +        return EINVAL;
> >> > > +    }
> >> > > +
> >>
> >> Please remove the check here, it is a static function and their API
> >> doesn't need to be checked.
> >>
> >> > >      size_t size;
> >> > >
> >> > >      if (len == 0) {
> >> > >          len = strlen(str);
> >> > > -    } else {
> >> > > +    }
> >> > > +    else {
> >>
> >> Also please revert this change, we use the following coding style in
> >> SSSD:
> >>
> >> if () {
> >> } else {
> >> }
> >>
> >> > >          while (len > 0 && str[len - 1] == '\0') len--;
> >> > >      }
> >> > >
> >> > > @@ -121,6 +141,9 @@ static errno_t sss_authtok_set_string(struct
> >> > > sss_auth_token *tok,
> >> > >
> >> > >  void sss_authtok_set_empty(struct sss_auth_token *tok)
> >> > >  {
> >> > > +    if (!tok) {
> >> > > +        return;
> >> > > +    }
> >> > >      switch (tok->type) {
> >> > >      case SSS_AUTHTOK_TYPE_EMPTY:
> >> > >          return;
> >> > > @@ -174,6 +197,9 @@ errno_t sss_authtok_set(struct sss_auth_token
> >> > > *tok,
> >> > >  errno_t sss_authtok_copy(struct sss_auth_token *src,
> >> > >                           struct sss_auth_token *dst)
> >> > >  {
> >> > > +    if (!src || !dst) {
> >> > > +        return EINVAL;
> >> > > +    }
> >> > >      sss_authtok_set_empty(dst);
> >> > >
> >> > >      if (src->type == SSS_AUTHTOK_TYPE_EMPTY) {
> >> > > @@ -205,7 +231,7 @@ struct sss_auth_token
> *sss_authtok_new(TALLOC_CTX
> >> > > *mem_ctx)
> >> > >
> >> > >  void sss_authtok_wipe_password(struct sss_auth_token *tok)
> >> > >  {
> >> > > -    if (tok->type != SSS_AUTHTOK_TYPE_PASSWORD) {
> >> > > +    if (!tok || tok->type != SSS_AUTHTOK_TYPE_PASSWORD) {
> >> > >          return;
> >> > >      }
> >> >
> >> > Aside from the q. of whether EFAULT may be more appropriate in this
> case
> >> > the rest looks sane.
> >>
> >> Looks good to me too.
> >> _______________________________________________
> >> sssd-devel mailing list
> >> sssd-devel at lists.fedorahosted.org
> >> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>
>
> >From 813e6cf4b275b44af6b4a2c56cca63a8366f4224 Mon Sep 17 00:00:00 2001
> >From: Pallavi Jha <pallavikumarijha at gmail.com>
> >Date: Sat, 16 Nov 2013 16:23:01 +0530
> >Subject: [PATCH] added null checks to authtok module
> >
> >---
> > src/util/authtok.c | 23 ++++++++++++++++++++++-
> > 1 file changed, 22 insertions(+), 1 deletion(-)
> >
> >diff --git a/src/util/authtok.c b/src/util/authtok.c
> >index
> 83e6a1c942a0abc136422bf062be47cbc687bb72..389e50504d943263c72e47a4d11dc818c2816565
> 100644
> >--- a/src/util/authtok.c
> >+++ b/src/util/authtok.c
> >@@ -32,6 +32,9 @@ enum sss_authtok_type sss_authtok_get_type(struct
> sss_auth_token *tok)
> >
> > size_t sss_authtok_get_size(struct sss_auth_token *tok)
> > {
> >+    if (!tok) {
> >+        return 0;
> >+    }
> >     switch (tok->type) {
> >     case SSS_AUTHTOK_TYPE_PASSWORD:
> >     case SSS_AUTHTOK_TYPE_CCFILE:
> >@@ -45,12 +48,18 @@ size_t sss_authtok_get_size(struct sss_auth_token
> *tok)
> >
> > uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
> > {
> >+    if (!tok) {
> >+        return NULL;
> >+    }
> >     return tok->data;
> > }
> >
> > errno_t sss_authtok_get_password(struct sss_auth_token *tok,
> >                                  const char **pwd, size_t *len)
> > {
> >+    if (!tok) {
> >+        return EFAULT;
> >+    }
> >     switch (tok->type) {
> >     case SSS_AUTHTOK_TYPE_EMPTY:
> >         return ENOENT;
> >@@ -70,6 +79,9 @@ errno_t sss_authtok_get_password(struct sss_auth_token
> *tok,
> > errno_t sss_authtok_get_ccfile(struct sss_auth_token *tok,
> >                                const char **ccfile, size_t *len)
> > {
> >+    if (!tok) {
> >+        return EINVAL;
> >+    }
> >     switch (tok->type) {
> >     case SSS_AUTHTOK_TYPE_EMPTY:
> >         return ENOENT;
> >@@ -91,6 +103,9 @@ static errno_t sss_authtok_set_string(struct
> sss_auth_token *tok,
> >                                       const char *context_name,
> >                                       const char *str, size_t len)
> > {
> >+    if (!tok || !str) {
> >+        return EINVAL;
> >+    }
>      ^^^^^^^^^^^^^^^^^^^^
> You probably missed Jakub's comment in the previous mail.
>
> >>Please remove the check here, it is a static function and their API
> >>doesn't need to be checked.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131208/9f96fb99/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-added-null-checks-to-authtok-module.patch
Type: text/x-patch
Size: 2497 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131208/9f96fb99/attachment.bin>


More information about the sssd-devel mailing list