krb5_free_data

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

krb5_free_data

Markus Moeller
Hi,

   I wonder if it wouldn't be  better to check if val->data is NULL before
freeing ?  To me the function makes otherwise no sense as I have to do the
same checks myself before  I can call the function safely.


void KRB5_CALLCONV
krb5_free_data(krb5_context context, krb5_data *val)
{
    if (val == NULL)
        return;
    free(val->data);
    free(val);
}


Thank you
Markus


_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: krb5_free_data

Simo Sorce-3
On Sat, 2014-06-21 at 17:56 +0100, Markus Moeller wrote:
> Hi,
>
>    I wonder if it wouldn't be  better to check if val->data is NULL before
> freeing ?

No, it would be useless, free(NULL); is ok, it is just a no-op.

Simo.

>   To me the function makes otherwise no sense as I have to do the
> same checks myself before  I can call the function safely.
>
>
> void KRB5_CALLCONV
> krb5_free_data(krb5_context context, krb5_data *val)
> {
>     if (val == NULL)
>         return;
>     free(val->data);
>     free(val);
> }
>
>
> Thank you
> Markus
>
>
> _______________________________________________
> krbdev mailing list             [hidden email]
> https://mailman.mit.edu/mailman/listinfo/krbdev


--
Simo Sorce * Red Hat, Inc * New York

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: krb5_free_data

Markus Moeller
Sorry I do not understand. Right now I get crashes as val->data is NULL. To
avoid I have to do exactly the same as the function should have done. I
expect the below.

void KRB5_CALLCONV
krb5_free_data(krb5_context context, krb5_data *val)
{
    if (val == NULL)
        return;
    If (val->data)
       free(val->data);
    free(val);
}

"Simo Sorce"  wrote in message
news:[hidden email]...

On Sat, 2014-06-21 at 17:56 +0100, Markus Moeller wrote:
> Hi,
>
>    I wonder if it wouldn't be  better to check if val->data is NULL before
> freeing ?

No, it would be useless, free(NULL); is ok, it is just a no-op.

Simo.

>   To me the function makes otherwise no sense as I have to do the
> same checks myself before  I can call the function safely.
>
>
> void KRB5_CALLCONV
> krb5_free_data(krb5_context context, krb5_data *val)
> {
>     if (val == NULL)
>         return;
>     free(val->data);
>     free(val);
> }
>
>
> Thank you
> Markus
>
>
> _______________________________________________
> krbdev mailing list             [hidden email]
> https://mailman.mit.edu/mailman/listinfo/krbdev


--
Simo Sorce * Red Hat, Inc * New York

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev 


_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: krb5_free_data

Greg Hudson
On 06/21/2014 05:17 PM, Markus Moeller wrote:
> Sorry I do not understand. Right now I get crashes as val->data is NULL.

free(NULL) does not crash.  See section 7.20.3.2 of:

    http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

or:

    http://pubs.opengroup.org/onlinepubs/009695399/functions/free.html

We make this assumption all over the krb5 code base, as do many other
programs.  Perhaps the crashes you are seeing have another cause?
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: krb5_free_data

Markus Moeller

Hi Greg,

I see this in krb5 1.8.3

(gdb) where
#0  0x00007ffff5f911b5 in *__GI_raise (sig=<value optimized out>) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x00007ffff5f93fc0 in *__GI_abort () at abort.c:92
#2  0x00007ffff5fc75bb in __libc_message (do_abort=<value optimized out>,
fmt=<value optimized out>) at ../sysdeps/unix/sysv/linux/libc_fatal.c:189
#3  0x00007ffff5fd0e16 in malloc_printerr (action=3, str=0x7ffff6088748
"double free or corruption (fasttop)", ptr=<value optimized out>)
    at malloc.c:6267
#4  0x00007ffff5fd5b8c in *__GI___libc_free (mem=<value optimized out>) at
malloc.c:3739
#5  0x00007ffff7939472 in krb5_free_data (context=<value optimized out>,
val=0x6171f0) at ../../../../src/lib/krb5/krb/kfree.c:253
#6  0x00000000004051a4 in get_ad_groups (ad_groups=0x7fffffffaff0 "",
context=0x60f9e0, pac=0x0) at negotiate_kerberos_pac.cc:464
#7  0x0000000000403265 in main (argc=5, argv=0x7fffffffe0e8) at
negotiate_kerberos_auth.cc:419

and line 253 is free(val->data)

The code I have is the following and I get an error in krb5_pac_get_buffer:
Invalid argument


ad_data = (krb5_data *)xmalloc(sizeof(krb5_data));

#define KERB_LOGON_INFO 1
    ret = krb5_pac_get_buffer(context, pac, KERB_LOGON_INFO, ad_data);
    if (check_k5_err(context, "krb5_pac_get_buffer", ret))
        goto k5clean;


k5clean:
    krb5_free_data(context, ad_data);

Maybe there is something wrong. I usually do not get this error, but I don't
know why it happens sometimes.

Markus

-----Original Message-----
From: Greg Hudson
Sent: Saturday, June 21, 2014 11:36 PM
To: Markus Moeller ; [hidden email]
Subject: Re: krb5_free_data

On 06/21/2014 05:17 PM, Markus Moeller wrote:
> Sorry I do not understand. Right now I get crashes as val->data is NULL.

free(NULL) does not crash.  See section 7.20.3.2 of:

    http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

or:

    http://pubs.opengroup.org/onlinepubs/009695399/functions/free.html

We make this assumption all over the krb5 code base, as do many other
programs.  Perhaps the crashes you are seeing have another cause?


_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: krb5_free_data

Tom Yu
Markus Moeller <[hidden email]> writes:

> (gdb) where
> #0  0x00007ffff5f911b5 in *__GI_raise (sig=<value optimized out>) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #1  0x00007ffff5f93fc0 in *__GI_abort () at abort.c:92
> #2  0x00007ffff5fc75bb in __libc_message (do_abort=<value optimized out>,
> fmt=<value optimized out>) at ../sysdeps/unix/sysv/linux/libc_fatal.c:189
> #3  0x00007ffff5fd0e16 in malloc_printerr (action=3, str=0x7ffff6088748
> "double free or corruption (fasttop)", ptr=<value optimized out>)
>     at malloc.c:6267
> #4  0x00007ffff5fd5b8c in *__GI___libc_free (mem=<value optimized out>) at
> malloc.c:3739
> #5  0x00007ffff7939472 in krb5_free_data (context=<value optimized out>,
> val=0x6171f0) at ../../../../src/lib/krb5/krb/kfree.c:253
> #6  0x00000000004051a4 in get_ad_groups (ad_groups=0x7fffffffaff0 "",
> context=0x60f9e0, pac=0x0) at negotiate_kerberos_pac.cc:464
> #7  0x0000000000403265 in main (argc=5, argv=0x7fffffffe0e8) at
> negotiate_kerberos_auth.cc:419
>
> and line 253 is free(val->data)
>
> The code I have is the following and I get an error in krb5_pac_get_buffer:
> Invalid argument
>
>
> ad_data = (krb5_data *)xmalloc(sizeof(krb5_data));
>
> #define KERB_LOGON_INFO 1
>     ret = krb5_pac_get_buffer(context, pac, KERB_LOGON_INFO, ad_data);
>     if (check_k5_err(context, "krb5_pac_get_buffer", ret))
>         goto k5clean;
>
>
> k5clean:
>     krb5_free_data(context, ad_data);
>
> Maybe there is something wrong. I usually do not get this error, but I don't
> know why it happens sometimes.

It appears that krb5_pac_get_buffer() does not initialize its output
parameter on error conditions.  This is probably a bug.  As a
workaround, consider using calloc() to allocate ad_data so that the
pointer gets initialized to NULL.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: krb5_free_data

Markus Moeller
Hi Tom,

I see.  So if k5_pac_locate_buffer returns with an error I may get this
problem ?

krb5_error_code KRB5_CALLCONV
krb5_pac_get_buffer(krb5_context context,
                    krb5_pac pac,
                    krb5_ui_4 type,
                    krb5_data *data)
{
    krb5_data d;
    krb5_error_code ret;

    ret = k5_pac_locate_buffer(context, pac, type, &d);
    if (ret != 0)
        return ret;

    data->data = malloc(d.length);
    if (data->data == NULL)
        return ENOMEM;
...


Thank you
Markus


"Tom Yu"  wrote in message news:[hidden email]...

Markus Moeller <[hidden email]> writes:

> (gdb) where
> #0  0x00007ffff5f911b5 in *__GI_raise (sig=<value optimized out>) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #1  0x00007ffff5f93fc0 in *__GI_abort () at abort.c:92
> #2  0x00007ffff5fc75bb in __libc_message (do_abort=<value optimized out>,
> fmt=<value optimized out>) at ../sysdeps/unix/sysv/linux/libc_fatal.c:189
> #3  0x00007ffff5fd0e16 in malloc_printerr (action=3, str=0x7ffff6088748
> "double free or corruption (fasttop)", ptr=<value optimized out>)
>     at malloc.c:6267
> #4  0x00007ffff5fd5b8c in *__GI___libc_free (mem=<value optimized out>) at
> malloc.c:3739
> #5  0x00007ffff7939472 in krb5_free_data (context=<value optimized out>,
> val=0x6171f0) at ../../../../src/lib/krb5/krb/kfree.c:253
> #6  0x00000000004051a4 in get_ad_groups (ad_groups=0x7fffffffaff0 "",
> context=0x60f9e0, pac=0x0) at negotiate_kerberos_pac.cc:464
> #7  0x0000000000403265 in main (argc=5, argv=0x7fffffffe0e8) at
> negotiate_kerberos_auth.cc:419
>
> and line 253 is free(val->data)
>
> The code I have is the following and I get an error in
> krb5_pac_get_buffer:
> Invalid argument
>
>
> ad_data = (krb5_data *)xmalloc(sizeof(krb5_data));
>
> #define KERB_LOGON_INFO 1
>     ret = krb5_pac_get_buffer(context, pac, KERB_LOGON_INFO, ad_data);
>     if (check_k5_err(context, "krb5_pac_get_buffer", ret))
>         goto k5clean;
>
>
> k5clean:
>     krb5_free_data(context, ad_data);
>
> Maybe there is something wrong. I usually do not get this error, but I
> don't
> know why it happens sometimes.

It appears that krb5_pac_get_buffer() does not initialize its output
parameter on error conditions.  This is probably a bug.  As a
workaround, consider using calloc() to allocate ad_data so that the
pointer gets initialized to NULL.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev 


_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: krb5_free_data

Greg Hudson
On 06/22/2014 08:04 AM, Markus Moeller wrote:
> I see.  So if k5_pac_locate_buffer returns with an error I may get this
> problem ?

Your code begins with:

>> ad_data = (krb5_data *)xmalloc(sizeof(krb5_data));

After this line, ad_data->data and ad_data->length are uninitialized.
If you were to immediately call krb5_free_data(context, ad_data) without
doing anything else, you would often see a crash because ad_data->data
is heap garbage.

If you use calloc instead of malloc, or call "memset(ad_data, 0,
sizeof(krb5_data))" after allocating, then you avoid this state.
(Strictly speaking, the C standard says that zeroed memory and null
pointers aren't necessarily the same thing, but lots of C code ignores
this and assumes that they are.  See http://c-faq.com/null/ for more on
that.)

Your code goes on to call:

>> #define KERB_LOGON_INFO 1
>>     ret = krb5_pac_get_buffer(context, pac, KERB_LOGON_INFO, ad_data);

If krb5_pac_get_buffer succeeds, ad_data->data and ad_data->length will
be filled in with the results, and you can safely free ad_data.

If it fails, it currently doesn't touch ad_data at all, leaving it
uninitialized and therefore unsafe to free.  Tom refers to this as
"probably a bug," but it's not uncommon for older libkrb5 APIs.  To be
safe, do not assume that libkrb5 APIs will initialize their output
parameters when they fail.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev
Reply | Threaded
Open this post in threaded view
|

Re: krb5_free_data

Markus Moeller
Thank you. I will need to review a lot of code then :-(
Markus

"Greg Hudson"  wrote in message news:[hidden email]...

On 06/22/2014 08:04 AM, Markus Moeller wrote:
> I see.  So if k5_pac_locate_buffer returns with an error I may get this
> problem ?

Your code begins with:

>> ad_data = (krb5_data *)xmalloc(sizeof(krb5_data));

After this line, ad_data->data and ad_data->length are uninitialized.
If you were to immediately call krb5_free_data(context, ad_data) without
doing anything else, you would often see a crash because ad_data->data
is heap garbage.

If you use calloc instead of malloc, or call "memset(ad_data, 0,
sizeof(krb5_data))" after allocating, then you avoid this state.
(Strictly speaking, the C standard says that zeroed memory and null
pointers aren't necessarily the same thing, but lots of C code ignores
this and assumes that they are.  See http://c-faq.com/null/ for more on
that.)

Your code goes on to call:

>> #define KERB_LOGON_INFO 1
>>     ret = krb5_pac_get_buffer(context, pac, KERB_LOGON_INFO, ad_data);

If krb5_pac_get_buffer succeeds, ad_data->data and ad_data->length will
be filled in with the results, and you can safely free ad_data.

If it fails, it currently doesn't touch ad_data at all, leaving it
uninitialized and therefore unsafe to free.  Tom refers to this as
"probably a bug," but it's not uncommon for older libkrb5 APIs.  To be
safe, do not assume that libkrb5 APIs will initialize their output
parameters when they fail.
_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev

_______________________________________________
krbdev mailing list             [hidden email]
https://mailman.mit.edu/mailman/listinfo/krbdev