[Patch] Cmocka unit test patch for authtok module

[Pallavi Jha]

Hi

I have created the patch and attached it with this mail. Kindly review it.
I have commented some of the test as it fails(gives segmentation fault)
when authtoken is NULL. I think we should test tok for NULL before
accessing its elements. Please correct me if am wrong.

for example :

uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
{
    return tok->data;
}

This should be written as:

uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
{
    if(!tok) {
        return EINVAL;
    }
    return tok->data;
}

Thanking You,
Pallavi

[Jakub Hrozek]

On Sat, Nov 09, 2013 at 07:29:46PM +0530, Pallavi Jha wrote:
>  Hi
>  
>  I have created the patch and attached it with this mail. 
I don't see any attachment, sorry..

>  Kindly review it.
>  I have commented some of the test as it fails(gives segmentation fault)
>  when authtoken is NULL. I think we should test tok for NULL before
>  accessing its elements. Please correct me if am wrong. 
I think you're right about the additional check, but you can't return
EINVAL, the return value of the functions is a pointer. You could return
NULL as the value instead:

uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
{
    if (!tok) {
        return NULL;
    }

    return tok->data;
}

Feel free to send the additional NULL check as another patch to the
list..

>  
>  for example :
>  
>  uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
>  {
>      return tok->data;
>  }
>  
>  This should be written as:
>  
>  uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
>  {
>      if(!tok) {
>          return EINVAL;
>      }
>      return tok->data;
>  }

[Lukas Slebodnik]

On (09/11/13 19:29), Pallavi Jha wrote:
> Hi
>
> I have created the patch and attached it with this mail. Kindly review it.
> I have commented some of the test as it fails(gives segmentation fault)
> when authtoken is NULL. I think we should test tok for NULL before
> accessing its elements. Please correct me if am wrong.
>
> for example :
>
> uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
> {
>     return tok->data;
> }
>
> This should be written as:
>
> uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
> {
>     if(!tok) {
>         return EINVAL;
>     }
>     return tok->data;
> }
>
> Thanking You,
> Pallavi 
I cannot find any attachment.

LS

[Pallavi Jha]

Hello,

I am extremely sorry for the inconvenience. The patch is attached with the
mail. Kindly have a look. I will shortly send another patch for authtor
module with the suggested fix.

Thanking You,
Pallavi

On 11 November 2013 17:05, Lukas Slebodnik <lslebodn(a)redhat.com&gt; wrote:

>  On (09/11/13 19:29), Pallavi Jha wrote:
>  >Hi
>  >
>  >I have created the patch and attached it with this mail. Kindly review it.
>  >I have commented some of the test as it fails(gives segmentation fault)
>  >when authtoken is NULL. I think we should test tok for NULL before
>  >accessing its elements. Please correct me if am wrong.
>  >
>  >for example :
>  >
>  >uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
>  >{
>  >    return tok->data;
>  >}
>  >
>  >This should be written as:
>  >
>  >uint8_t *sss_authtok_get_data(struct sss_auth_token *tok)
>  >{
>  >    if(!tok) {
>  >        return EINVAL;
>  >    }
>  >    return tok->data;
>  >}
>  >
>  >Thanking You,
>  >Pallavi
>
>  I cannot find any attachment.
>
>  LS
>

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/09/2013 08:59 AM, Pallavi Jha wrote:
>  Hi
>  
>  I have created the patch and attached it with this mail. Kindly
>  review it. I have commented some of the test as it fails(gives
>  segmentation fault) when authtoken is NULL. I think we should test
>  tok for NULL before accessing its elements. Please correct me if am
>  wrong.
>  
>  for example :
>  
>  uint8_t *sss_authtok_get_data(struct sss_auth_token *tok) { return
>  tok->data; }
>  
>  This should be written as:
>  
>  uint8_t *sss_authtok_get_data(struct sss_auth_token *tok) { 
>  if(!tok) { return EINVAL; } return tok->data; }
>  
>  Thanking You, Pallavi 

I think you may have forgotten to attach the patch, Pallavi.

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.15 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlKA3kwACgkQeiVVYja6o6ONTwCgheuQ1ZLfNN79nSFR9O64R8Qo
> L54An12Fp14jEThaNW2EM1OJxtBdbpta
> =mdMQ
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

On Mon, Nov 11, 2013 at 05:23:04PM +0530, Pallavi Jha wrote:
>  Hello,
>  
>  I am extremely sorry for the inconvenience. The patch is attached with the
>  mail. Kindly have a look. I will shortly send another patch for authtor
>  module with the suggested fix.
>  
>  Thanking You,
>  Pallavi 
This code is a good start, thank you. It needs some changes before it can be
accepted, but we can fix them quite simply.

I noticed you use setup and teardown functions with most tests, but not
test_sss_authtok_new(). I think you can use the setup and teardown
everywhere and simplify the code a bit, because the "state" pointer can
already act as a talloc context. By using the setup and teardown, the
test could be simplified to:

static void test_sss_authtok_new(void **state)
{
    struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
    struct sss_auth_token *authtoken;

    authtoken = sss_authtok_new(ts);
    assert_non_null(authtoken);

    talloc_free(authtoken);
}

Notice there's no talloc called here, we just use the "struct
test_state". Another interation of the patch might also check for leaks
as well, but let's not worry about that now.

Also, the test_sss_authtok_new() attempts to return ENOMEM, even though it's
a void-typed function. In the test cases that are void-typed you should use
the assert macros instead, so if you need to allocate memory next time, you can
do:
    mem_ctx = talloc_new(parent_ctx);
    assert_non_null(mem_ctx);

The same issue is in test_sss_authtok_copy().

About test_sss_authtok() and test_sss_authtok_set() -- I think it would
make sense to combine them into a single test case or if you prefer,
one test case per authtok type.

The flow in the test case is fine, I think, the case assigns some data
and then retrieves them back with getter and compares, which is the
point. I'm just thinking that having the tests per authtok type might
make them more readable, then each case would have the same flow that
would cover the whole "lifecycle" of the authtok:

In the same test, this code is not necessary:
    size_t *ret_len = (size_t *)malloc(sizeof(size_t *));
    const char **pwd = (const char **)malloc(sizeof(const char **));
   
    ret = sss_authtok_get_password(ts->authtoken, pwd, ret_len);

You can just use local variables:
    size_t ret_len;
    const char *pwd;
   
    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);

But please move declarations of all variables to the top of the function
(that's just a style choice of the SSSD).

And lastly -- please remove the tests with NULL authtok from the current
test and submit them only when they work along with the improvement to
return error on NULL authtok.

Thank you very much for the contribution!

[Pallavi Jha]

Hi,

I have made the required changes in the patch attached with the mail.
Kindly review it.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 11 November 2013 20:24, Jakub Hrozek <jhrozek(a)redhat.com&gt; wrote:

>  On Mon, Nov 11, 2013 at 05:23:04PM +0530, Pallavi Jha wrote:
>  > Hello,
>  >
>  > I am extremely sorry for the inconvenience. The patch is attached with
>  the
>  > mail. Kindly have a look. I will shortly send another patch for authtor
>  > module with the suggested fix.
>  >
>  > Thanking You,
>  > Pallavi
>
>  This code is a good start, thank you. It needs some changes before it can
>  be
>  accepted, but we can fix them quite simply.
>
>  I noticed you use setup and teardown functions with most tests, but not
>  test_sss_authtok_new(). I think you can use the setup and teardown
>  everywhere and simplify the code a bit, because the "state" pointer can
>  already act as a talloc context. By using the setup and teardown, the
>  test could be simplified to:
>
>  static void test_sss_authtok_new(void **state)
>  {
>      struct test_state *ts = talloc_get_type_abort(*state, struct
>  test_state);
>      struct sss_auth_token *authtoken;
>
>      authtoken = sss_authtok_new(ts);
>      assert_non_null(authtoken);
>
>      talloc_free(authtoken);
>  }
>
>  Notice there's no talloc called here, we just use the "struct
>  test_state". Another interation of the patch might also check for leaks
>  as well, but let's not worry about that now.
>
>  Also, the test_sss_authtok_new() attempts to return ENOMEM, even though
>  it's
>  a void-typed function. In the test cases that are void-typed you should use
>  the assert macros instead, so if you need to allocate memory next time,
>  you can
>  do:
>      mem_ctx = talloc_new(parent_ctx);
>      assert_non_null(mem_ctx);
>
>  The same issue is in test_sss_authtok_copy().
>
>  About test_sss_authtok() and test_sss_authtok_set() -- I think it would
>  make sense to combine them into a single test case or if you prefer,
>  one test case per authtok type.
>
>  The flow in the test case is fine, I think, the case assigns some data
>  and then retrieves them back with getter and compares, which is the
>  point. I'm just thinking that having the tests per authtok type might
>  make them more readable, then each case would have the same flow that
>  would cover the whole "lifecycle" of the authtok:
>
>  In the same test, this code is not necessary:
>      size_t *ret_len = (size_t *)malloc(sizeof(size_t *));
>      const char **pwd = (const char **)malloc(sizeof(const char **));
>
>      ret = sss_authtok_get_password(ts->authtoken, pwd, ret_len);
>
>  You can just use local variables:
>      size_t ret_len;
>      const char *pwd;
>
>      ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>
>  But please move declarations of all variables to the top of the function
>  (that's just a style choice of the SSSD).
>
>  And lastly -- please remove the tests with NULL authtok from the current
>  test and submit them only when they work along with the improvement to
>  return error on NULL authtok.
>
>  Thank you very much for the contribution!
>

[Jakub Hrozek]

On Tue, Nov 12, 2013 at 04:53:23PM +0530, Pallavi Jha wrote:
>  Hi,
>  
>  I have made the required changes in the patch attached with the mail.
>  Kindly review it.
>  
>  With regards
>  Pallavi Kumari Jha
>  Computer Science Engineering
>   
Hi,

this version is much better. I only have a few comments now, see below:

>  new file mode 100644
>  index 0000000000000000000000000000000000000000..87e3c25ae7992ee48916725b87e616db42827462
>  --- /dev/null
>  +++ b/src/tests/cmocka/test_authtok.c
>  @@ -0,0 +1,309 @@
>  +/*
>  +    SSSD
>  +
>  +    authtok - Utilities tests
>  +
>  +    Authors:
>  +        Pallavi Jha <pallavikumarijha(a)gmail.com&gt;
>  +
>  +    Copyright (C) 2013 Red Hat
>  +
>  +    This program is free software; you can redistribute it and/or modify
>  +    it under the terms of the GNU General Public License as published by
>  +    the Free Software Foundation; either version 3 of the License, or
>  +    (at your option) any later version.
>  +
>  +    This program is distributed in the hope that it will be useful,
>  +    but WITHOUT ANY WARRANTY; without even the implied warranty of
>  +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  +    GNU General Public License for more details.
>  +
>  +    You should have received a copy of the GNU General Public License
>  +    along with this program.  If not, see <http://www.gnu.org/licenses/>;.
>  +*/
>  +
>  +#include <stdarg.h>
>  +#include <stddef.h>
>  +#include <setjmp.h>
>  +#include <cmocka.h>
>  +#include <string.h>
>  +
>  +#include "util/authtok.h"
>  +
>  +
>  +struct test_state
>  +{ 
Can you make the above "struct test_state {" ? That would match the
rest of the SSSD code better..

>  +    struct sss_auth_token *authtoken;
>  +};
>  +
>  +static void setup(void **state)
>  +{
>  +    struct test_state *ts = NULL;
>  +
>  +    ts = talloc(NULL, struct test_state);
>  +    assert_non_null(ts);
>  +
>  +    ts->authtoken = sss_authtok_new(ts);
>  +    assert_non_null(ts->authtoken);
>  +
>  +    *state = (void *)ts;
>  +}
>  +
>  +static void teardown(void **state)
>  +{
>  +    struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
>  +    talloc_free(ts);
>  +}
>  +
>  +static void test_sss_authtok_new(void **state)
>  +{
>  +    struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
>  +    struct sss_auth_token *authtoken;
>  +
>  +    authtoken = sss_authtok_new(ts);
>  +    assert_non_null(authtoken);
>  +
>  +    talloc_free(authtoken);
>  +}
>  +
>  +/* @test_authtok_type_x : tests following functions for different value of type
>  + * sss_authtok_set
>  + * sss_authtok_get_type
>  + * sss_authtok_get_size
>  + * sss_authtok_get_data
>  + * sss_authtok_get_password
>  + * sss_authtok_get_ccfile
>  + *
>  + * @test_authtok_type_1 : type => SSS_AUTHTOK_TYPE_PASSWORD 
I think you can rename the functions to test_authtok_passwd,
test_authtok_ccfile and test_authtok_empty. The names would be more
descriptive

>  + * @test_authtok_type_2 : type => SSS_AUTHTOK_TYPE_CCFILE
>  + * @test_authtok_type_3 : type => SSS_AUTHTOK_TYPE_EMPTY
>  + */
>  +
>  +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */
>  +static void test_sss_authtok_type_1(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    uint8_t *data;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  +
>  +    data = strdup("password"); 
"data" being uint8_t* actually causes a compilation warning here:
/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_authtok.c:93:10: warning: assigning
to 'uint8_t *'
      (aka 'unsigned char *') from 'char *' converts between pointers to
integer types with different
      sign [-Wpointer-sign]
    data = strdup("password");

I would suggest to compile with stricter CFLAGS to see the warnings.

I think given we know password is a string here, we can just use "const char *"
as type of "data" and then typcast to uint8_t* just when passing the password
to sss_authtok_set

>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, data, len); 
This line would then say:
  ret = sss_authtok_set(ts->authtoken, type, (uint8_t *) data, len);

>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  + 
>  +    assert_int_equal(ret, EACCES); 
This is technically correct, but I think we should just test for the return
code being non-EOK, with:
       assert_int_not_equal(ret, EOK);

>  +
>  +    ret = sss_authtok_set(ts->authtoken, type, data, 0); 
I think it would make sense to also test sss_authtok_set_password in
this function in addition to the more generic sss_authtok_set. Similarly
for other tests, call both sss_authtok_set with the right type and also
sss_authtok_set_$type.

>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len); 
I didn't understand why you called sss_authtok_get_ccfile() twice on the
same authtoken? Was one of them supposed to be _get_empty?

>  + 
>  +    assert_int_equal(ret, EACCES);
>  +}
>  +
>  +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */
>  +static void test_sss_authtok_type_2(void **state) 
Same comment about renaming the function.

>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    uint8_t *data; 
Same comment about data type here.

>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  + 
>  +    data = strdup("password"); 
The content of authtok of type CCFILE is usually a path to a Kerberos
credentials file. Can you change the example from password to something that
looks like a path? (It doesn't have to be real, feel free to use something like
"/path/to/cc_file")

> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_CCFILE;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> + 
> +    assert_int_equal(ret, EACCES);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
>  +
>  +    ret = sss_authtok_set(ts->authtoken, type, data, 0); 
> + 
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> + 
> +    assert_int_equal(ret, EACCES);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +}
> +
> +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
> +static void test_sss_authtok_type_3(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    uint8_t *data; 
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  + 
>  +    data = strdup("password"); 
> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_EMPTY;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);

I think you can safely omit "data" and "len" from this call.

> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> +    assert_null(sss_authtok_get_data(ts->authtoken));
> + 
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> + 
> +    ret = sss_authtok_set(ts->authtoken, type, '\0', 0);
> +    assert_int_equal(ret, EOK);
> +
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> +    assert_null(sss_authtok_get_data(ts->authtoken));
> + 
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +}
> +
> +static void test_sss_authtok_wipe_password(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    uint8_t *data; 
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  + 
>  +    data = strdup("password"); 
>  +    len =
> strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, data, len); 
> +
> +    assert_int_equal(ret, EOK);
> +
> +    sss_authtok_wipe_password(ts->authtoken);
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> + 
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(pwd, "");
> +    assert_int_equal(len - 1, ret_len);

Hm, I think you actually might have found a bug here. I find it suprising
that after wiping the password the length is retained..I think the function
should set length to zero. I'll file a ticket about this and discuss with the
other developers.

> +}
> +
> +static void test_sss_authtok_copy(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    uint8_t *data; 
> +    TALLOC_CTX *mem_ctx;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> +    struct sss_auth_token *dest_authtoken;
> + 
> +    mem_ctx = talloc_new(NULL);

I think you can use "ts" as talloc context instead of NULL here.

>  +    assert_non_null(mem_ctx);
>  + 
>  +    ts= talloc_get_type_abort(*state, struct test_state);
>  + 
>  +    dest_authtoken = sss_authtok_new(mem_ctx);
>  +    assert_non_null(dest_authtoken);
>  +
>  +    data = strdup("password");
>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_EMPTY;
>  +    ret = sss_authtok_set(ts->authtoken, type, data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(EOK, sss_authtok_copy(ts->authtoken, dest_authtoken));
>  +
>  +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  +    ret = sss_authtok_set(ts->authtoken, type, data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +
>  +    ret = sss_authtok_copy(ts->authtoken, dest_authtoken);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(dest_authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(dest_authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(dest_authtoken));
>  +
>  +    talloc_free(mem_ctx);
>  +}
>  +
>  +int main(void)
>  +{
>  +    const UnitTest tests[] = {
>  +        unit_test_setup_teardown(test_sss_authtok_new, setup, teardown),
>  +        unit_test_setup_teardown(test_sss_authtok_type_1, setup, teardown),
>  +        unit_test_setup_teardown(test_sss_authtok_type_2, setup, teardown),
>  +        unit_test_setup_teardown(test_sss_authtok_type_3, setup, teardown),
>  +        unit_test_setup_teardown(test_sss_authtok_wipe_password, setup,
>  +                                 teardown),
>  +        unit_test_setup_teardown(test_sss_authtok_copy, setup, teardown)
>  +    };
>  +
>  +    return run_tests(tests);
>  +} 
Thanks for the patch! You're making progress and hopefully we'll be able to accept
the
next version.

[Jakub Hrozek]

On Wed, Nov 13, 2013 at 03:46:38PM +0100, Jakub Hrozek wrote:
>  > +#include "util/authtok.h"
>  > +
>  > +
>  > +struct test_state
>  > +{
>  
>  Can you make the above "struct test_state {" ? That would match the
>  rest of the SSSD code better.. 
To clarify further (sorry I missed your ping on the IRC today), what I
requested is simply a formatting change. In SSSD, we use the curly
bracket on the same line as the struct keyword when defining structures.
So instead of:
 struct test_state
 {
     struct sss_auth_token *authtoken;
 };

you'd use:
 struct test_state {
     struct sss_auth_token *authtoken;
 };

[Pallavi Jha]

Patch with the suggested fix is attached along with the mail. Kindly
review.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 15 November 2013 00:10, Jakub Hrozek <jhrozek(a)redhat.com&gt; wrote:

>  On Wed, Nov 13, 2013 at 03:46:38PM +0100, Jakub Hrozek wrote:
>  > > +#include "util/authtok.h"
>  > > +
>  > > +
>  > > +struct test_state
>  > > +{
>  >
>  > Can you make the above "struct test_state {" ? That would match the
>  > rest of the SSSD code better..
>
>  To clarify further (sorry I missed your ping on the IRC today), what I
>  requested is simply a formatting change. In SSSD, we use the curly
>  bracket on the same line as the struct keyword when defining structures.
>  So instead of:
>   struct test_state
>   {
>       struct sss_auth_token *authtoken;
>   };
>
>  you'd use:
>   struct test_state {
>       struct sss_auth_token *authtoken;
>   };
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

[Jakub Hrozek]

On Fri, Nov 15, 2013 at 10:44:21PM +0530, Pallavi Jha wrote:
>  Patch with the suggested fix is attached along with the mail. Kindly
>  review. 
Hi Pallavi,

I spotted a couple more places that need fixing, but in overall the test
looks OK and runs fine. Nice job, let's fix the last small issues, see
inline:

>  +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */
>  +static void test_sss_authtok_password(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    const char *data;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  +
>  +    data = strdup("password"); 
Instead of strdup it's more convenient to use talloc_strdup here:
    ts = talloc_get_type_abort(*state, struct test_state);
    data = talloc_strdup(ts, "password");
    assert_non_null(data);

The advantage is that when the teardown function frees test_state, the
data will be freed as well, so you could use valgrind to check for
leaks.

>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  +    ret = sss_authtok_set_password(ts->authtoken, data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, 0);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken)); 
I think you can omit the sss_authtok_set and the checks above. I think
you can rely on sss_authtok_set_password() to do its job and just call
the sss_authtok_get_password() below to ensure the password was set
correctly with sss_authtok_set_password().

>  +
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +}
>  +
>  +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */
>  +static void test_sss_authtok_ccfile(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    const char *data;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  + 
>  +    data = strdup("path/to/cc_file"); 
Same comment about strdup. Sorry I didn't catch it earlier.

>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_CCFILE;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  +    ret = sss_authtok_set_ccfile(ts->authtoken, data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  + 
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  + 
I think the checks below are not necessary, are they? You already test
sss_authtok_set with sss_authtok_get_ccfile and also
sss_authtok_set_ccfile with sss_authtok_get_ccfile.

Maybe I'm not seeing something, but why another sss_authtok_set and
sss_authtok_get_ccfile?

>  +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
>  + 
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +}
>  +
>  +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
>  +static void test_sss_authtok_empty(void **state)
>  +{
>  +    errno_t ret;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  + 
>  +    type = SSS_AUTHTOK_TYPE_EMPTY;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, NULL, 0);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>  +    assert_null(sss_authtok_get_data(ts->authtoken));
>  + 
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, ENOENT);
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len); 
There is a compilation warning triggered by the get_ccfile call:
warning: incompatible pointer types passing 'const char *' to parameter
of type 'const char **'; take the address with &
      [-Wincompatible-pointer-types]
warning: incompatible integer to pointer conversion passing 'size_t'
(aka 'unsigned long') to parameter of type 'size_t *'
      (aka 'unsigned long *'); take the address with &
      [-Wint-conversion]

And it's right, you need to pass &pwd, &ret_len to avoid the warning.

The same warning is also triggered by the last sss_authtok_get_ccfile()
call at the end of the function.

>  +
>  +    assert_int_equal(ret, ENOENT);
>  +
>  +    sss_authtok_set_empty(ts->authtoken);
>  + 
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>  +    assert_null(sss_authtok_get_data(ts->authtoken));
>  + 
>  +    ret = sss_authtok_set(ts->authtoken, type, '\0', 0);
>  +    assert_int_equal(ret, EOK);
>  +
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken)); 
Please test for EOK, not 0 here. They are the same right now, but might
not be in the future.

>  +    assert_null(sss_authtok_get_data(ts->authtoken));
>  + 
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, ENOENT);
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
>  +
>  +    assert_int_equal(ret, ENOENT);
>  +}
>  +
>  +static void test_sss_authtok_wipe_password(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    uint8_t *data;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  + 
>  +    data = strdup("password"); 
Please also use talloc_strdup here. I am seeing one more warning here:
warning: assigning to 'uint8_t *' (aka 'unsigned char *') from 'char
*'
converts between pointers to integer types with different sign
      [-Wpointer-sign]

So when you convert the strdup to talloc_strdup, you can just cast the
result to uint8_t:
      data = (uint8_t *) talloc_strdup(ts, "password");

Or alternatively change the pointer type to const char * and cast to
uint8_t* when passing the pointer to sss_authtok_set(). This option
might be better as that's what you do in the other tests.

>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +
>  +    sss_authtok_wipe_password(ts->authtoken);
>  +
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  + 
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(pwd, "");
>  +    assert_int_equal(len - 1, ret_len);
>  +}
>  +
>  +static void test_sss_authtok_copy(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    const char *data;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  +    struct sss_auth_token *dest_authtoken;
>  + 
>  +    ts= talloc_get_type_abort(*state, struct test_state);
>  + 
>  +    dest_authtoken = sss_authtok_new(ts);
>  +    assert_non_null(dest_authtoken);
>  +
>  +    data = strdup("password"); 
Please use talloc_strdup here as well.

>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_EMPTY;
>  +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(EOK, sss_authtok_copy(ts->authtoken, dest_authtoken)); 
Did you maybe want to also check the type here after you called
sss_authtok_copy?
        assert_int_equal(type, sss_authtok_get_type(dest_authtoken));

>  +
>  +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +
>  +    ret = sss_authtok_copy(ts->authtoken, dest_authtoken);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(dest_authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(dest_authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(dest_authtoken)); 
Thanks for keeping up during the review!

[Pallavi Jha]

Hello Jakub,

Yes sure, will make the required changes and send the patch shortly.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 19 November 2013 16:06, Jakub Hrozek <jhrozek(a)redhat.com&gt; wrote:

>  On Fri, Nov 15, 2013 at 10:44:21PM +0530, Pallavi Jha wrote:
>  > Patch with the suggested fix is attached along with the mail. Kindly
>  > review.
>
>  Hi Pallavi,
>
>  I spotted a couple more places that need fixing, but in overall the test
>  looks OK and runs fine. Nice job, let's fix the last small issues, see
>  inline:
>
>  > +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */
>  > +static void test_sss_authtok_password(void **state)
>  > +{
>  > +    size_t len;
>  > +    errno_t ret;
>  > +    const char *data;
>  > +    size_t ret_len;
>  > +    const char *pwd;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>  > +
>  > +    data = strdup("password");
>
>  Instead of strdup it's more convenient to use talloc_strdup here:
>      ts = talloc_get_type_abort(*state, struct test_state);
>      data = talloc_strdup(ts, "password");
>      assert_non_null(data);
>
>  The advantage is that when the teardown function frees test_state, the
>  data will be freed as well, so you could use valgrind to check for
>  leaks.
>
>  > +    len = strlen(data) + 1;
>  > +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  > +    ts = talloc_get_type_abort(*state, struct test_state);
>  > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +
>  > +    ret = sss_authtok_set_password(ts->authtoken, data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +
>  > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, 0);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>
>  I think you can omit the sss_authtok_set and the checks above. I think
>  you can rely on sss_authtok_set_password() to do its job and just call
>  the sss_authtok_get_password() below to ensure the password was set
>  correctly with sss_authtok_set_password().
>
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +}
>  > +
>  > +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */
>  > +static void test_sss_authtok_ccfile(void **state)
>  > +{
>  > +    size_t len;
>  > +    errno_t ret;
>  > +    const char *data;
>  > +    size_t ret_len;
>  > +    const char *pwd;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>  > +
>  > +    data = strdup("path/to/cc_file");
>
>  Same comment about strdup. Sorry I didn't catch it earlier.
>
>  > +    len = strlen(data) + 1;
>  > +    type = SSS_AUTHTOK_TYPE_CCFILE;
>  > +    ts = talloc_get_type_abort(*state, struct test_state);
>  > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +
>  > +    ret = sss_authtok_set_ccfile(ts->authtoken, data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +
>  > +
>
>  I think the checks below are not necessary, are they? You already test
>  sss_authtok_set with sss_authtok_get_ccfile and also
>  sss_authtok_set_ccfile with sss_authtok_get_ccfile.
>
>  Maybe I'm not seeing something, but why another sss_authtok_set and
>  sss_authtok_get_ccfile?
>
>  > +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +}
>  > +
>  > +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
>  > +static void test_sss_authtok_empty(void **state)
>  > +{
>  > +    errno_t ret;
>  > +    size_t ret_len;
>  > +    const char *pwd;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>  > +
>  > +    type = SSS_AUTHTOK_TYPE_EMPTY;
>  > +    ts = talloc_get_type_abort(*state, struct test_state);
>  > +    ret = sss_authtok_set(ts->authtoken, type, NULL, 0);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>  > +    assert_null(sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, ENOENT);
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
>
>  There is a compilation warning triggered by the get_ccfile call:
>  warning: incompatible pointer types passing 'const char *' to parameter
>  of type 'const char **'; take the address with &
>        [-Wincompatible-pointer-types]
>  warning: incompatible integer to pointer conversion passing 'size_t'
>  (aka 'unsigned long') to parameter of type 'size_t *'
>        (aka 'unsigned long *'); take the address with &
>        [-Wint-conversion]
>
>  And it's right, you need to pass &pwd, &ret_len to avoid the warning.
>
>  The same warning is also triggered by the last sss_authtok_get_ccfile()
>  call at the end of the function.
>
>  > +
>  > +    assert_int_equal(ret, ENOENT);
>  > +
>  > +    sss_authtok_set_empty(ts->authtoken);
>  > +
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>  > +    assert_null(sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_set(ts->authtoken, type, '\0', 0);
>  > +    assert_int_equal(ret, EOK);
>  > +
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>
>  Please test for EOK, not 0 here. They are the same right now, but might
>  not be in the future.
>
>  > +    assert_null(sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, ENOENT);
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
>  > +
>  > +    assert_int_equal(ret, ENOENT);
>  > +}
>  > +
>  > +static void test_sss_authtok_wipe_password(void **state)
>  > +{
>  > +    size_t len;
>  > +    errno_t ret;
>  > +    uint8_t *data;
>  > +    size_t ret_len;
>  > +    const char *pwd;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>  > +
>  > +    data = strdup("password");
>
>  Please also use talloc_strdup here. I am seeing one more warning here:
>  warning: assigning to 'uint8_t *' (aka 'unsigned char *') from 'char
> *'
>  converts between pointers to integer types with different sign
>        [-Wpointer-sign]
>
>  So when you convert the strdup to talloc_strdup, you can just cast the
>  result to uint8_t:
>        data = (uint8_t *) talloc_strdup(ts, "password");
>
>  Or alternatively change the pointer type to const char * and cast to
>  uint8_t* when passing the pointer to sss_authtok_set(). This option
>  might be better as that's what you do in the other tests.
>
>  > +    len = strlen(data) + 1;
>  > +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  > +    ts = talloc_get_type_abort(*state, struct test_state);
>  > +    ret = sss_authtok_set(ts->authtoken, type, data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +
>  > +    sss_authtok_wipe_password(ts->authtoken);
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(pwd, "");
>  > +    assert_int_equal(len - 1, ret_len);
>  > +}
>  > +
>  > +static void test_sss_authtok_copy(void **state)
>  > +{
>  > +    size_t len;
>  > +    errno_t ret;
>  > +    const char *data;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>  > +    struct sss_auth_token *dest_authtoken;
>  > +
>  > +    ts= talloc_get_type_abort(*state, struct test_state);
>  > +
>  > +    dest_authtoken = sss_authtok_new(ts);
>  > +    assert_non_null(dest_authtoken);
>  > +
>  > +    data = strdup("password");
>
>  Please use talloc_strdup here as well.
>
>  > +    len = strlen(data) + 1;
>  > +    type = SSS_AUTHTOK_TYPE_EMPTY;
>  > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(EOK, sss_authtok_copy(ts->authtoken,
>  dest_authtoken));
>
>  Did you maybe want to also check the type here after you called
>  sss_authtok_copy?
>          assert_int_equal(type, sss_authtok_get_type(dest_authtoken));
>
>  > +
>  > +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +
>  > +    ret = sss_authtok_copy(ts->authtoken, dest_authtoken);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(dest_authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(dest_authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(dest_authtoken));
>
>  Thanks for keeping up during the review!
>

[Pallavi Jha]

For  your comment

I think the checks below are not necessary, are they? You already test
sss_authtok_set with sss_authtok_get_ccfile and also
sss_authtok_set_ccfile with sss_authtok_get_ccfile.

Maybe I'm not seeing something, but why another sss_authtok_set and
sss_authtok_get_ccfile?

>  +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_ccfile(ts-> 
authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +}
>  +
>  +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */ 

Here I have tested what will happen when length is zero. Previously it was
not tested. I have made the changes. Kindly review the patch.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 22 November 2013 01:11, Pallavi Jha <pallavikumarijha(a)gmail.com&gt; wrote:

>  Hello Jakub,
>
>  Yes sure, will make the required changes and send the patch shortly.
>
>  With regards
>  Pallavi Kumari Jha
>  Computer Science Engineering
>
>
>  On 19 November 2013 16:06, Jakub Hrozek <jhrozek(a)redhat.com&gt; wrote:
>
> > On Fri, Nov 15, 2013 at 10:44:21PM +0530, Pallavi Jha wrote:
> > > Patch with the suggested fix is attached along with the mail. Kindly
> > > review.
> >
> > Hi Pallavi,
> >
> > I spotted a couple more places that need fixing, but in overall the test
> > looks OK and runs fine. Nice job, let's fix the last small issues, see
> > inline:
> >
> > > +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */
> > > +static void test_sss_authtok_password(void **state)
> > > +{
> > > +    size_t len;
> > > +    errno_t ret;
> > > +    const char *data;
> > > +    size_t ret_len;
> > > +    const char *pwd;
> > > +    struct test_state *ts;
> > > +    enum sss_authtok_type type;
> > > +
> > > +    data = strdup("password");
> >
> > Instead of strdup it's more convenient to use talloc_strdup here:
> >     ts = talloc_get_type_abort(*state, struct test_state);
> >     data = talloc_strdup(ts, "password");
> >     assert_non_null(data);
> >
> > The advantage is that when the teardown function frees test_state, the
> > data will be freed as well, so you could use valgrind to check for
> > leaks.
> >
> > > +    len = strlen(data) + 1;
> > > +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> > > +    ts = talloc_get_type_abort(*state, struct test_state);
> > > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> > > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> > > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> > > +
> > > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_string_equal(data, pwd);
> > > +    assert_int_equal(len - 1, ret_len);
> > > +
> > > +    ret = sss_authtok_set_password(ts->authtoken, data, len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_string_equal(data, pwd);
> > > +    assert_int_equal(len - 1, ret_len);
> > > +
> > > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, 0);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> > > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> > > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> >
> > I think you can omit the sss_authtok_set and the checks above. I think
> > you can rely on sss_authtok_set_password() to do its job and just call
> > the sss_authtok_get_password() below to ensure the password was set
> > correctly with sss_authtok_set_password().
> >
> > > +
> > > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_string_equal(data, pwd);
> > > +    assert_int_equal(len - 1, ret_len);
> > > +}
> > > +
> > > +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */
> > > +static void test_sss_authtok_ccfile(void **state)
> > > +{
> > > +    size_t len;
> > > +    errno_t ret;
> > > +    const char *data;
> > > +    size_t ret_len;
> > > +    const char *pwd;
> > > +    struct test_state *ts;
> > > +    enum sss_authtok_type type;
> > > +
> > > +    data = strdup("path/to/cc_file");
> >
> > Same comment about strdup. Sorry I didn't catch it earlier.
> >
> > > +    len = strlen(data) + 1;
> > > +    type = SSS_AUTHTOK_TYPE_CCFILE;
> > > +    ts = talloc_get_type_abort(*state, struct test_state);
> > > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> > > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> > > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> > > +
> > > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_string_equal(data, pwd);
> > > +    assert_int_equal(len - 1, ret_len);
> > > +
> > > +    ret = sss_authtok_set_ccfile(ts->authtoken, data, len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +
> > > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_string_equal(data, pwd);
> > > +    assert_int_equal(len - 1, ret_len);
> > > +
> > > +
> >
> > I think the checks below are not necessary, are they? You already test
> > sss_authtok_set with sss_authtok_get_ccfile and also
> > sss_authtok_set_ccfile with sss_authtok_get_ccfile.
> >
> > Maybe I'm not seeing something, but why another sss_authtok_set and
> > sss_authtok_get_ccfile?
> >
> > > +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> > > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> > > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> > > +
> > > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_string_equal(data, pwd);
> > > +    assert_int_equal(len - 1, ret_len);
> > > +}
> > > +
> > > +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
> > > +static void test_sss_authtok_empty(void **state)
> > > +{
> > > +    errno_t ret;
> > > +    size_t ret_len;
> > > +    const char *pwd;
> > > +    struct test_state *ts;
> > > +    enum sss_authtok_type type;
> > > +
> > > +    type = SSS_AUTHTOK_TYPE_EMPTY;
> > > +    ts = talloc_get_type_abort(*state, struct test_state);
> > > +    ret = sss_authtok_set(ts->authtoken, type, NULL, 0);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> > > +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> > > +    assert_null(sss_authtok_get_data(ts->authtoken));
> > > +
> > > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> > > +
> > > +    assert_int_equal(ret, ENOENT);
> > > +
> > > +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
> >
> > There is a compilation warning triggered by the get_ccfile call:
> > warning: incompatible pointer types passing 'const char *' to parameter
> > of type 'const char **'; take the address with &
> >       [-Wincompatible-pointer-types]
> > warning: incompatible integer to pointer conversion passing 'size_t'
> > (aka 'unsigned long') to parameter of type 'size_t *'
> >       (aka 'unsigned long *'); take the address with &
> >       [-Wint-conversion]
> >
> > And it's right, you need to pass &pwd, &ret_len to avoid the warning.
> >
> > The same warning is also triggered by the last sss_authtok_get_ccfile()
> > call at the end of the function.
> >
> > > +
> > > +    assert_int_equal(ret, ENOENT);
> > > +
> > > +    sss_authtok_set_empty(ts->authtoken);
> > > +
> > > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> > > +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> > > +    assert_null(sss_authtok_get_data(ts->authtoken));
> > > +
> > > +    ret = sss_authtok_set(ts->authtoken, type, '\0', 0);
> > > +    assert_int_equal(ret, EOK);
> > > +
> > > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> > > +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> >
> > Please test for EOK, not 0 here. They are the same right now, but might
> > not be in the future.
> >
> > > +    assert_null(sss_authtok_get_data(ts->authtoken));
> > > +
> > > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> > > +
> > > +    assert_int_equal(ret, ENOENT);
> > > +
> > > +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
> > > +
> > > +    assert_int_equal(ret, ENOENT);
> > > +}
> > > +
> > > +static void test_sss_authtok_wipe_password(void **state)
> > > +{
> > > +    size_t len;
> > > +    errno_t ret;
> > > +    uint8_t *data;
> > > +    size_t ret_len;
> > > +    const char *pwd;
> > > +    struct test_state *ts;
> > > +    enum sss_authtok_type type;
> > > +
> > > +    data = strdup("password");
> >
> > Please also use talloc_strdup here. I am seeing one more warning here:
> > warning: assigning to 'uint8_t *' (aka 'unsigned char *') from
> 'char *'
> > converts between pointers to integer types with different sign
> >       [-Wpointer-sign]
> >
> > So when you convert the strdup to talloc_strdup, you can just cast the
> > result to uint8_t:
> >       data = (uint8_t *) talloc_strdup(ts, "password");
> >
> > Or alternatively change the pointer type to const char * and cast to
> > uint8_t* when passing the pointer to sss_authtok_set(). This option
> > might be better as that's what you do in the other tests.
> >
> > > +    len = strlen(data) + 1;
> > > +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> > > +    ts = talloc_get_type_abort(*state, struct test_state);
> > > +    ret = sss_authtok_set(ts->authtoken, type, data, len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +
> > > +    sss_authtok_wipe_password(ts->authtoken);
> > > +
> > > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_string_equal(pwd, "");
> > > +    assert_int_equal(len - 1, ret_len);
> > > +}
> > > +
> > > +static void test_sss_authtok_copy(void **state)
> > > +{
> > > +    size_t len;
> > > +    errno_t ret;
> > > +    const char *data;
> > > +    struct test_state *ts;
> > > +    enum sss_authtok_type type;
> > > +    struct sss_auth_token *dest_authtoken;
> > > +
> > > +    ts= talloc_get_type_abort(*state, struct test_state);
> > > +
> > > +    dest_authtoken = sss_authtok_new(ts);
> > > +    assert_non_null(dest_authtoken);
> > > +
> > > +    data = strdup("password");
> >
> > Please use talloc_strdup here as well.
> >
> > > +    len = strlen(data) + 1;
> > > +    type = SSS_AUTHTOK_TYPE_EMPTY;
> > > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_int_equal(EOK, sss_authtok_copy(ts->authtoken,
> > dest_authtoken));
> >
> > Did you maybe want to also check the type here after you called
> > sss_authtok_copy?
> >         assert_int_equal(type, sss_authtok_get_type(dest_authtoken));
> >
> > > +
> > > +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> > > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +
> > > +    ret = sss_authtok_copy(ts->authtoken, dest_authtoken);
> > > +
> > > +    assert_int_equal(ret, EOK);
> > > +    assert_int_equal(type, sss_authtok_get_type(dest_authtoken));
> > > +    assert_string_equal(data, sss_authtok_get_data(dest_authtoken));
> > > +    assert_int_equal(len, sss_authtok_get_size(dest_authtoken));
> >
> > Thanks for keeping up during the review!
> >
>
>

[Jakub Hrozek]

On Sun, Dec 08, 2013 at 01:36:15AM +0545, Pallavi Jha wrote:
>  For  your comment
>  
>  I think the checks below are not necessary, are they? You already test
>  sss_authtok_set with sss_authtok_get_ccfile and also
>  sss_authtok_set_ccfile with sss_authtok_get_ccfile.
>  
>  Maybe I'm not seeing something, but why another sss_authtok_set and
>  sss_authtok_get_ccfile?
>  
>  > +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->
>  authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +}
>  > +
>  > +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
>  
>  
>  Here I have tested what will happen when length is zero. Previously it was
>  not tested. I have made the changes. Kindly review the patch. 
Ah, thanks for explanation.

>  
>  With regards
>  Pallavi Kumari Jha
>  Computer Science Engineering 
There is still some changes that need to be done, the current version of
the patch crashes for me. See below:


>  +/* @test_authtok_type_x : tests following functions for different
> value of type
>  + * sss_authtok_set
>  + * sss_authtok_get_type
>  + * sss_authtok_get_size
>  + * sss_authtok_get_data
>  + * sss_authtok_get_password
>  + * sss_authtok_get_ccfile
>  + *
>  + * @test_authtok_type_password : type => SSS_AUTHTOK_TYPE_PASSWORD
>  + * @test_authtok_type_ccfile   : type => SSS_AUTHTOK_TYPE_CCFILE
>  + * @test_authtok_type_empty    : type => SSS_AUTHTOK_TYPE_EMPTY
>  + */
>  +
>  +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */
>  +static void test_sss_authtok_password(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    const char *data;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  +
>  +    data = talloc_strdup(ts, "password"); 
Here you use "ts" before initializing it. You need to call
talloc_get_type_abort before the first use of ts.

>  +    assert_non_null(data);
>  +
>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  +    ret = sss_authtok_set_password(ts->authtoken, data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +}
>  +
>  +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */
>  +static void test_sss_authtok_ccfile(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    const char *data;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  + 
>  +    data = talloc_strdup(ts, "path/to/cc_file"); 
Here ts is used uninitialized as well.

>  +    assert_non_null(data);
>  +
>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_CCFILE;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  +    ret = sss_authtok_set_ccfile(ts->authtoken, data, len); 
This line was giving me a compilation warning because data is const char
and set_ccfile expects uint8_t. You can simple cast data to uint8_t *

    ret = sss_authtok_set_ccfile(ts->authtoken, (uint8_t *) data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  + 
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +
>  +
>  +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
>  + 
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(data, pwd);
>  +    assert_int_equal(len - 1, ret_len);
>  +}
>  +
>  +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
>  +static void test_sss_authtok_empty(void **state)
>  +{
>  +    errno_t ret;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type;
>  + 
>  +    type = SSS_AUTHTOK_TYPE_EMPTY;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, NULL, 0);
>  +
>  +    assert_int_equal(ret, EOK);
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>  +    assert_null(sss_authtok_get_data(ts->authtoken));
>  + 
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, ENOENT);
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, ENOENT);
>  +
>  +    sss_authtok_set_empty(ts->authtoken);
>  + 
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>  +    assert_null(sss_authtok_get_data(ts->authtoken));
>  + 
>  +    ret = sss_authtok_set(ts->authtoken, type, '\0', 0);
>  +    assert_int_equal(ret, EOK);
>  +
>  +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  +    assert_int_equal(EOK, sss_authtok_get_size(ts->authtoken));
>  +    assert_null(sss_authtok_get_data(ts->authtoken));
>  + 
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, ENOENT);
>  +
>  +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  +
>  +    assert_int_equal(ret, ENOENT);
>  +}
>  +
>  +static void test_sss_authtok_wipe_password(void **state)
>  +{
>  +    size_t len;
>  +    errno_t ret;
>  +    const char *data;
>  +    size_t ret_len;
>  +    const char *pwd;
>  +    struct test_state *ts;
>  +    enum sss_authtok_type type; 
And also here.

>  + 
>  +    data = talloc_strdup(ts, "password");
>  +    assert_non_null(data);
>  +
>  +    len = strlen(data) + 1;
>  +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  +    ts = talloc_get_type_abort(*state, struct test_state);
>  +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  +
>  +    assert_int_equal(ret, EOK);
>  +
>  +    sss_authtok_wipe_password(ts->authtoken);
>  +
>  +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  + 
>  +    assert_int_equal(ret, EOK);
>  +    assert_string_equal(pwd, "");
>  +    assert_int_equal(len - 1, ret_len);
>  +}
>  +

[Pallavi Jha]

Hello,

Please find the updated patch attached with this mail.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 9 December 2013 19:32, Jakub Hrozek <jhrozek(a)redhat.com&gt; wrote:

>  On Sun, Dec 08, 2013 at 01:36:15AM +0545, Pallavi Jha wrote:
>  > For  your comment
>  >
>  > I think the checks below are not necessary, are they? You already test
>  > sss_authtok_set with sss_authtok_get_ccfile and also
>  > sss_authtok_set_ccfile with sss_authtok_get_ccfile.
>  >
>  > Maybe I'm not seeing something, but why another sss_authtok_set and
>  > sss_authtok_get_ccfile?
>  >
>  > > +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
>  > > +
>  > > +    assert_int_equal(ret, EOK);
>  > > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  > > +
>  > > +    ret = sss_authtok_get_ccfile(ts->
>  > authtoken, &pwd, &ret_len);
>  > > +
>  > > +    assert_int_equal(ret, EOK);
>  > > +    assert_string_equal(data, pwd);
>  > > +    assert_int_equal(len - 1, ret_len);
>  > > +}
>  > > +
>  > > +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
>  >
>  >
>  > Here I have tested what will happen when length is zero. Previously it
>  was
>  > not tested. I have made the changes. Kindly review the patch.
>
>  Ah, thanks for explanation.
>
>  >
>  > With regards
>  > Pallavi Kumari Jha
>  > Computer Science Engineering
>
>  There is still some changes that need to be done, the current version of
>  the patch crashes for me. See below:
>
>
>  > +/* @test_authtok_type_x : tests following functions for different value
>  of type
>  > + * sss_authtok_set
>  > + * sss_authtok_get_type
>  > + * sss_authtok_get_size
>  > + * sss_authtok_get_data
>  > + * sss_authtok_get_password
>  > + * sss_authtok_get_ccfile
>  > + *
>  > + * @test_authtok_type_password : type => SSS_AUTHTOK_TYPE_PASSWORD
>  > + * @test_authtok_type_ccfile   : type => SSS_AUTHTOK_TYPE_CCFILE
>  > + * @test_authtok_type_empty    : type => SSS_AUTHTOK_TYPE_EMPTY
>  > + */
>  > +
>  > +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */
>  > +static void test_sss_authtok_password(void **state)
>  > +{
>  > +    size_t len;
>  > +    errno_t ret;
>  > +    const char *data;
>  > +    size_t ret_len;
>  > +    const char *pwd;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>  > +
>  > +    data = talloc_strdup(ts, "password");
>
>  Here you use "ts" before initializing it. You need to call
>  talloc_get_type_abort before the first use of ts.
>
>  > +    assert_non_null(data);
>  > +
>  > +    len = strlen(data) + 1;
>  > +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  > +    ts = talloc_get_type_abort(*state, struct test_state);
>  > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +
>  > +    ret = sss_authtok_set_password(ts->authtoken, data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +}
>  > +
>  > +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */
>  > +static void test_sss_authtok_ccfile(void **state)
>  > +{
>  > +    size_t len;
>  > +    errno_t ret;
>  > +    const char *data;
>  > +    size_t ret_len;
>  > +    const char *pwd;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>  > +
>  > +    data = talloc_strdup(ts, "path/to/cc_file");
>
>  Here ts is used uninitialized as well.
>
>  > +    assert_non_null(data);
>  > +
>  > +    len = strlen(data) + 1;
>  > +    type = SSS_AUTHTOK_TYPE_CCFILE;
>  > +    ts = talloc_get_type_abort(*state, struct test_state);
>  > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +
>  > +    ret = sss_authtok_set_ccfile(ts->authtoken, data, len);
>
>  This line was giving me a compilation warning because data is const char
>  and set_ccfile expects uint8_t. You can simple cast data to uint8_t *
>
>      ret = sss_authtok_set_ccfile(ts->authtoken, (uint8_t *) data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +
>  > +
>  > +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
>  > +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(data, pwd);
>  > +    assert_int_equal(len - 1, ret_len);
>  > +}
>  > +
>  > +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
>  > +static void test_sss_authtok_empty(void **state)
>  > +{
>  > +    errno_t ret;
>  > +    size_t ret_len;
>  > +    const char *pwd;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>  > +
>  > +    type = SSS_AUTHTOK_TYPE_EMPTY;
>  > +    ts = talloc_get_type_abort(*state, struct test_state);
>  > +    ret = sss_authtok_set(ts->authtoken, type, NULL, 0);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>  > +    assert_null(sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, ENOENT);
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, ENOENT);
>  > +
>  > +    sss_authtok_set_empty(ts->authtoken);
>  > +
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
>  > +    assert_null(sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_set(ts->authtoken, type, '\0', 0);
>  > +    assert_int_equal(ret, EOK);
>  > +
>  > +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
>  > +    assert_int_equal(EOK, sss_authtok_get_size(ts->authtoken));
>  > +    assert_null(sss_authtok_get_data(ts->authtoken));
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, ENOENT);
>  > +
>  > +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, ENOENT);
>  > +}
>  > +
>  > +static void test_sss_authtok_wipe_password(void **state)
>  > +{
>  > +    size_t len;
>  > +    errno_t ret;
>  > +    const char *data;
>  > +    size_t ret_len;
>  > +    const char *pwd;
>  > +    struct test_state *ts;
>  > +    enum sss_authtok_type type;
>
>  And also here.
>
>  > +
>  > +    data = talloc_strdup(ts, "password");
>  > +    assert_non_null(data);
>  > +
>  > +    len = strlen(data) + 1;
>  > +    type = SSS_AUTHTOK_TYPE_PASSWORD;
>  > +    ts = talloc_get_type_abort(*state, struct test_state);
>  > +    ret = sss_authtok_set(ts->authtoken, type, (uint8_t *)data, len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +
>  > +    sss_authtok_wipe_password(ts->authtoken);
>  > +
>  > +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
>  > +
>  > +    assert_int_equal(ret, EOK);
>  > +    assert_string_equal(pwd, "");
>  > +    assert_int_equal(len - 1, ret_len);
>  > +}
>  > +
>

[Jakub Hrozek]

On Wed, Dec 11, 2013 at 03:47:17PM +0545, Pallavi Jha wrote:
>  Hello,
>  
>  Please find the updated patch attached with this mail.
>  
>  With regards
>  Pallavi Kumari Jha
>  Computer Science Engineering 
Hi Pallavi,
there are still two compilation warnings in the code. Attached is a
patch with changes I suggest, can you check them and if you agree squash
them in your patch?

[Pallavi Jha]

Hi,

Thank you for the patch. I checked it and have included the changes in
the patch attached with this mail. Please look into it.

[Jakub Hrozek]

On Wed, Jan 08, 2014 at 01:18:58PM +0545, Pallavi Jha wrote:
>  Hi,
>  
>  Thank you for the patch. I checked it and have included the changes in
>  the patch attached with this mail. Please look into it. 
ACK

[Jakub Hrozek]

On Wed, Jan 08, 2014 at 11:27:05AM +0100, Jakub Hrozek wrote:
>  On Wed, Jan 08, 2014 at 01:18:58PM +0545, Pallavi Jha wrote:
>  > Hi,
>  > 
>  > Thank you for the patch. I checked it and have included the changes in
>  > the patch attached with this mail. Please look into it.
>  
>  ACK 
Pushed to master.