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