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