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@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@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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel