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;
+ }
size_t size;
if (len == 0) {
@@ -121,6 +136,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 +192,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 +226,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;
}
--
1.8.1.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
