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 <pallavikumari...@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