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(a)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.