[SSSD] [Patch] Cmocka unit test patch for authtok module

Pallavi Jha pallavikumarijha at gmail.com
Sat Dec 7 19:51:15 UTC 2013


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 at gmail.com> 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 at redhat.com> 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!
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131208/29d7411d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-cmocka-unit-test-for-authtok-module-added.patch
Type: text/x-patch
Size: 10834 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131208/29d7411d/attachment.bin>


More information about the sssd-devel mailing list