if calloc() needs nmemb and size, why doesn't freezero()?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
if calloc() and recallocarray() needs  nmemb and size, why doesn't
freezero()?

Should there be a freeczero(size_t nmemb, size_t size) ?

-Luke
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Theo de Raadt-2
Luke Small <[hidden email]> wrote:

> if calloc() and recallocarray() needs  nmemb and size, why doesn't
> freezero()?
>
> Should there be a freeczero(size_t nmemb, size_t size) ?

Performing the nmemb*size overflow detection a second time provides
no benefit.


Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
I guess I always thought there'd be some more substantial overflow
mitigation.

Would it be too much hand-holding to put in the manpage that to avoid
potential freeezero() integer overflow,
it may be useful to run freezero() as freezero((size_t)nmemb *
(size_t)size);

-Luke


On Wed, Feb 17, 2021 at 11:04 AM Theo de Raadt <[hidden email]> wrote:

> Luke Small <[hidden email]> wrote:
>
> > if calloc() and recallocarray() needs  nmemb and size, why doesn't
> > freezero()?
> >
> > Should there be a freeczero(size_t nmemb, size_t size) ?
>
> Performing the nmemb*size overflow detection a second time provides
> no benefit.
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Theo de Raadt-2
Luke Small <[hidden email]> wrote:

> I guess I always thought there'd be some more substantial overflow mitigation.

You have to free with the exact same size as allocation.

nmemb and size did not change.

The math has already been checked, and regular codeflows will store the
multiple in a single variable after successful checking&allocation, for
reuse.

> Would it be too much hand-holding to put in the manpage that to avoid potential
> freeezero() integer overflow,
> it may be useful to run freezero() as freezero((size_t)nmemb * (size_t)size);

Wow, Those casts make it very clear you don't understand C, if you do
that kind of stuff elsewhere you are introducing problems.

Sorry no you are wrong.

Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
I guess I’ve been doing it wrong all this time.

Perhaps you can tell me how the following doesn't return a 0-255 value.

uint64_t bufferToTime(const u_char buf[])
{

return (( (( (( (( (( (( ((
  (uint64_t) buf[7]) << 8)
                 | buf[6]) << 8)
                 | buf[5]) << 8)
                 | buf[4]) << 8)
                 | buf[3]) << 8)
                 | buf[2]) << 8)
                 | buf[1]) << 8)
                 | buf[0];
}

}

On Wed, Feb 17, 2021 at 12:05 PM Theo de Raadt <[hidden email]> wrote:

> Luke Small <[hidden email]> wrote:
>
> > I guess I always thought there'd be some more substantial overflow
> mitigation.
>
> You have to free with the exact same size as allocation.
>
> nmemb and size did not change.
>
> The math has already been checked, and regular codeflows will store the
> multiple in a single variable after successful checking&allocation, for
> reuse.
>
> > Would it be too much hand-holding to put in the manpage that to avoid
> potential
> > freeezero() integer overflow,
> > it may be useful to run freezero() as freezero((size_t)nmemb *
> (size_t)size);
>
> Wow, Those casts make it very clear you don't understand C, if you do
> that kind of stuff elsewhere you are introducing problems.
>
> Sorry no you are wrong.
>
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Theo de Raadt-2
>  > Would it be too much hand-holding to put in the manpage that to avoid potential
>  > freeezero() integer overflow,
>  > it may be useful to run freezero() as freezero((size_t)nmemb * (size_t)size);
>
>  Wow, Those casts make it very clear you don't understand C, if you do
>  that kind of stuff elsewhere you are introducing problems.

If nmemb or size are of a type greater than size_t, those casts serve only one
purpose -- truncating the high bits before performing multiply, which results in
an incorrect size.



Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
if the nmemb and size values being passed to calloc() are of a larger
integer datatype, they will have been truncated when passed to the function
there as well.

Perhaps you need something larger than size_t in the entire malloc manpage
series?



-Luke


On Wed, Feb 17, 2021 at 2:25 PM Theo de Raadt <[hidden email]> wrote:

> >  > Would it be too much hand-holding to put in the manpage that to avoid
> potential
> >  > freeezero() integer overflow,
> >  > it may be useful to run freezero() as freezero((size_t)nmemb *
> (size_t)size);
> >
> >  Wow, Those casts make it very clear you don't understand C, if you do
> >  that kind of stuff elsewhere you are introducing problems.
>
> If nmemb or size are of a type greater than size_t, those casts serve only
> one
> purpose -- truncating the high bits before performing multiply, which
> results in
> an incorrect size.
>
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
Am I incorrect to presume that if the values are successfully used in
calloc(), that (size_t)nmemb * (size_t)size will not overflow?
Isn't the storage capacity of size_t greater than the amount of addressable
space? If it is, calloc() will throw an "out of memory" or other error
before you'll ever reach putting freezero((size_t)nmemb * (size_t)size);

-Luke


On Wed, Feb 17, 2021 at 2:36 PM Luke Small <[hidden email]> wrote:

> if the nmemb and size values being passed to calloc() are of a larger
> integer datatype, they will have been truncated when passed to the function
> there as well.
>
> Perhaps you need something larger than size_t in the entire malloc manpage
> series?
>
>
>
> -Luke
>
>
> On Wed, Feb 17, 2021 at 2:25 PM Theo de Raadt <[hidden email]> wrote:
>
>> >  > Would it be too much hand-holding to put in the manpage that to
>> avoid potential
>> >  > freeezero() integer overflow,
>> >  > it may be useful to run freezero() as freezero((size_t)nmemb *
>> (size_t)size);
>> >
>> >  Wow, Those casts make it very clear you don't understand C, if you do
>> >  that kind of stuff elsewhere you are introducing problems.
>>
>> If nmemb or size are of a type greater than size_t, those casts serve
>> only one
>> purpose -- truncating the high bits before performing multiply, which
>> results in
>> an incorrect size.
>>
>>
>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Otto Moerbeek
In reply to this post by Theo de Raadt-2
On Wed, Feb 17, 2021 at 11:05:49AM -0700, Theo de Raadt wrote:

> Luke Small <[hidden email]> wrote:
>
> > I guess I always thought there'd be some more substantial overflow mitigation.
>
> You have to free with the exact same size as allocation.

Small correction: the size may be smaller than the original. In that
case, only a partial clear is guaranteed, the deallocation will still
be for the full allocation. Originally we were more strict, but iirc
that was causing to much headaches for some. See
https://cvsweb.openbsd.org/src/lib/libc/stdlib/malloc.c?rev=1.221

But the point stands: nmemb * size does not overflow, since the
original allocation would have overflowed and thus failed.

        -Otto

>
> nmemb and size did not change.
>
> The math has already been checked, and regular codeflows will store the
> multiple in a single variable after successful checking&allocation, for
> reuse.
>
> > Would it be too much hand-holding to put in the manpage that to avoid potential
> > freeezero() integer overflow,
> > it may be useful to run freezero() as freezero((size_t)nmemb * (size_t)size);
>
> Wow, Those casts make it very clear you don't understand C, if you do
> that kind of stuff elsewhere you are introducing problems.
>
> Sorry no you are wrong.
>

Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
However, calloc(ptr, nmemb, size) may have been called using smaller int
variable types which would overflow when multiplied. Where if the variables
storing the values passed to nmemb and size are less than or especially
equal to their original values, I think it’d be good to state that:

freezero(ptr, (size_t)nmemb * (size_t)size);
is guaranteed to work, but
freezero(ptr, nmemb * size);
does not have that guarantee.

On Thu, Feb 18, 2021 at 3:42 AM Otto Moerbeek <[hidden email]> wrote:

> On Wed, Feb 17, 2021 at 11:05:49AM -0700, Theo de Raadt wrote:
>
> > Luke Small <[hidden email]> wrote:
> >
> > > I guess I always thought there'd be some more substantial overflow
> mitigation.
> >
> > You have to free with the exact same size as allocation.
>
> Small correction: the size may be smaller than the original. In that
> case, only a partial clear is guaranteed, the deallocation will still
> be for the full allocation. Originally we were more strict, but iirc
> that was causing to much headaches for some. See
> https://cvsweb.openbsd.org/src/lib/libc/stdlib/malloc.c?rev=1.221
>
> But the point stands: nmemb * size does not overflow, since the
> original allocation would have overflowed and thus failed.
>
>         -Otto
>
> >
> > nmemb and size did not change.
> >
> > The math has already been checked, and regular codeflows will store the
> > multiple in a single variable after successful checking&allocation, for
> > reuse.
> >
> > > Would it be too much hand-holding to put in the manpage that to avoid
> potential
> > > freeezero() integer overflow,
> > > it may be useful to run freezero() as freezero((size_t)nmemb *
> (size_t)size);
> >
> > Wow, Those casts make it very clear you don't understand C, if you do
> > that kind of stuff elsewhere you are introducing problems.
> >
> > Sorry no you are wrong.
> >
>
--
-Luke
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Theo de Raadt-2
Luke Small <[hidden email]> wrote:

> However, calloc(ptr, nmemb, size) may have been called using smaller int
> variable types which would overflow when multiplied.

In which case the allocation would not have succeeded.

> Where if the variables
> storing the values passed to nmemb and size are less than or especially
> equal to their original values, I think it’d be good to state that:

Huh?

> freezero(ptr, (size_t)nmemb * (size_t)size);
> is guaranteed to work, but
> freezero(ptr, nmemb * size);
> does not have that guarantee.

I hope I never run any software by you.

Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
I had a drawn out email email describing passing by value and the
function’s need to only perform size_t multiplication overload checking but
not only do you not care I don’t think it’s worth my time to merely succeed
in angering you. I love your work!

On Thu, Feb 18, 2021 at 7:10 PM Theo de Raadt <[hidden email]> wrote:

> Luke Small <[hidden email]> wrote:
>
> > However, calloc(ptr, nmemb, size) may have been called using smaller int
> > variable types which would overflow when multiplied.
>
> In which case the allocation would not have succeeded.


> > Where if the variables
> > storing the values passed to nmemb and size are less than or especially
> > equal to their original values, I think it’d be good to state that:
>
> Huh?
>
> > freezero(ptr, (size_t)nmemb * (size_t)size);
> > is guaranteed to work, but
> > freezero(ptr, nmemb * size);
> > does not have that guarantee.
>
> I hope I never run any software by you.
>
--
-Luke
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Otto Moerbeek
In reply to this post by Luke Small
On Thu, Feb 18, 2021 at 03:24:36PM -0600, Luke Small wrote:

> However, calloc(ptr, nmemb, size) may have been called using smaller int
> variable types which would overflow when multiplied. Where if the variables
> storing the values passed to nmemb and size are less than or especially
> equal to their original values, I think it’d be good to state that:
>
> freezero(ptr, (size_t)nmemb * (size_t)size);
> is guaranteed to work, but
> freezero(ptr, nmemb * size);
> does not have that guarantee.

Lets try to make things explicit.

The function c() does the overflowe check like calloc does.
The function f() takes a size_t.

#include <limits.h>
#include <stdio.h>

#define MUL_NO_OVERFLOW (1UL << (sizeof(size_t) * 4))

void c(size_t nmemb, size_t size)
{
        if ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) &&
            nmemb > 0 && SIZE_T_MAX / nmemb < size)
                printf("Overflow\n");
        else
                printf("%zu\n", nmemb * size);
}

void f(size_t m)
{
        printf("%zu\n", m);
}

int
main()
{
        int a = INT_MAX;
        int b = INT_MAX;
        c(a, b);
        f(a * b);
}

Now the issues is that the multiplication in the last line of main()
overflows:

$ ./a.out
4611686014132420609
1

because this is an int multiplication only after that the promotion to
size_t is done.

So you are right that this can happen, *if you are using the wrong
types*. But I would argue that feeding anything other than either
size_t or constants to calloc() is already wrong. You *have* to
consider the argument conversion rules when feeding values to calloc()
(or any function). To avoid having to think about those, start with
size_t already for everything that is a size or count of a memory
object.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
malloc(3) already speaks to programmers who might use int multiplication
and telling them to test for int multiplication overflow in malloc(), so
you presume that they are already prepared to use something smaller than
size_t, when you could have just said:
“only use size_t variables for integer types.” and cut out the int
multiplication overflow test example.

In the manpage you could succinctly state:

In malloc(3):
“If you use smaller integer types than size_t for ‘nmemb’ and ‘size’, then
multiplication in freezero() may need to be cast to size_t to avoid integer
overflow:
freezero(ptr, (size_t)nmemb * (size_t)size);”
Or maybe even: freezero(ptr, (size_t)nmemb * size);

Or:

void freeczero( size_t nmemb, size_t size)
{
        freezero(nmemb * size);
}

I suspect that freezero() is already little more than:

void freezero(void *ptr, size_t size)
{
        explicit_bzero(ptr, size);
        free(ptr);
}

On Fri, Feb 19, 2021 at 12:51 AM Otto Moerbeek <[hidden email]> wrote:

> On Thu, Feb 18, 2021 at 03:24:36PM -0600, Luke Small wrote:
>
> > However, calloc(ptr, nmemb, size) may have been called using smaller int
> > variable types which would overflow when multiplied. Where if the
> variables
> > storing the values passed to nmemb and size are less than or especially
> > equal to their original values, I think it’d be good to state that:
> >
> > freezero(ptr, (size_t)nmemb * (size_t)size);
> > is guaranteed to work, but
> > freezero(ptr, nmemb * size);
> > does not have that guarantee.
>
> Lets try to make things explicit.
>
> The function c() does the overflowe check like calloc does.
> The function f() takes a size_t.
>
> #include <limits.h>
> #include <stdio.h>
>
> #define MUL_NO_OVERFLOW (1UL << (sizeof(size_t) * 4))
>
> void c(size_t nmemb, size_t size)
> {
>         if ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) &&
>             nmemb > 0 && SIZE_T_MAX / nmemb < size)
>                 printf("Overflow\n");
>         else
>                 printf("%zu\n", nmemb * size);
> }
>
> void f(size_t m)
> {
>         printf("%zu\n", m);
> }
>
> int
> main()
> {
>         int a = INT_MAX;
>         int b = INT_MAX;
>         c(a, b);
>         f(a * b);
> }
>
> Now the issues is that the multiplication in the last line of main()
> overflows:
>
> $ ./a.out
> 4611686014132420609
> 1
>
> because this is an int multiplication only after that the promotion to
> size_t is done.
>
> So you are right that this can happen, *if you are using the wrong
> types*. But I would argue that feeding anything other than either
> size_t or constants to calloc() is already wrong. You *have* to
> consider the argument conversion rules when feeding values to calloc()
> (or any function). To avoid having to think about those, start with
> size_t already for everything that is a size or count of a memory
> object.
>
>         -Otto
>
--
-Luke
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Theo de Raadt-2
Luke Small <[hidden email]> wrote:

> malloc(3) already speaks to programmers who might use int multiplication and telling
> them to test for int multiplication overflow in malloc(), so you presume that they are
> already prepared to use something smaller than size_t, when you could have just said:
> “only use size_t variables for integer types.” and cut out the int multiplication
> overflow test example.

It seems you don't understand C, and don't want to be taught.

> In the manpage you could succinctly state:
>
> In malloc(3):
> “If you use smaller integer types than size_t for ‘nmemb’ and ‘size’, then
> multiplication in freezero() may need to be cast to size_t to avoid integer overflow:
> freezero(ptr, (size_t)nmemb * (size_t)size);”
> Or maybe even: freezero(ptr, (size_t)nmemb * size);

That is incorrect.

> Or:
>
> void freeczero( size_t nmemb, size_t size)
> {
>         freezero(nmemb * size);
> }

Not going to happen.

> I suspect that freezero() is already little more than:
>
> void freezero(void *ptr, size_t size)
> {
>         explicit_bzero(ptr, size);
>         free(ptr);
> }

Wrong.

Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
>
> > In the manpage you could succinctly state:
> >
> > In malloc(3):
> > “If you use smaller integer types than size_t for ‘nmemb’ and ‘size’,
> then

> multiplication in freezero() may need to be cast to size_t to avoid
> integer overflow:
> > freezero(ptr, (size_t)nmemb * (size_t)size);”
> > Or maybe even: freezero(ptr, (size_t)nmemb * size);
>
> That is incorrect.


If it’s functionally incorrect, then tell me how the following cast acts
equivalently to intermediate storage or at least calls operations which act
upon uint64_t and makes the function return what is obviously intended on
your OpenBSD:

uint64_t bufferToTime(const u_char buf[])
{

return (( (( (( (( (( (( ((
  (uint64_t) buf[7]) << 8)
                 | buf[6]) << 8)
                 | buf[5]) << 8)
                 | buf[4]) << 8)
                 | buf[3]) << 8)
                 | buf[2]) << 8)
                 | buf[1]) << 8)
                 | buf[0];
}

Because it works.
--
-Luke
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Todd C. Miller-3
In reply to this post by Luke Small
On Fri, 19 Feb 2021 10:38:13 -0600, Luke Small wrote:

> In malloc(3):
> “If you use smaller integer types than size_t for ‘nmemb’ and ‘size’, then
> multiplication in freezero() may need to be cast to size_t to avoid integer
> overflow:
> freezero(ptr, (size_t)nmemb * (size_t)size);”
> Or maybe even: freezero(ptr, (size_t)nmemb * size);

This is bad advice.  The product of two size_t values can exceed
SIZE_MAX, at which point you would get integer overflow.  This is
why the malloc(3) man page warns against it.  Note that on 64-bit
platforms like amd64, size_t is already 64-bit so casting to unsigned
long long or uint64_t is not effective.

On OpenBSD, calloc(3) and reallocarray(3) check for integer overflow
for you, which is why they are preferred over malloc(nmemb * size).
You can examing the code yourself:
http://cvsweb.openbsd.org/src/lib/libc/stdlib/reallocarray.c?rev=1.3

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
I agree it can overflow. But if you use the same variables with the same
values plugged into

ptr = calloc(nmemb, size);

as you use in

freezero(ptr, (size_t)nmemb * size);

If it can overflow, it will have done it already in calloc().


On Fri, Feb 19, 2021 at 12:23 PM Todd C. Miller <[hidden email]> wrote:

> On Fri, 19 Feb 2021 10:38:13 -0600, Luke Small wrote:
>
> > In malloc(3):
> > “If you use smaller integer types than size_t for ‘nmemb’ and ‘size’,
> then
> > multiplication in freezero() may need to be cast to size_t to avoid
> integer
> > overflow:
> > freezero(ptr, (size_t)nmemb * (size_t)size);”
> > Or maybe even: freezero(ptr, (size_t)nmemb * size);
>
> This is bad advice.  The product of two size_t values can exceed
> SIZE_MAX, at which point you would get integer overflow.  This is
> why the malloc(3) man page warns against it.  Note that on 64-bit
> platforms like amd64, size_t is already 64-bit so casting to unsigned
> long long or uint64_t is not effective.
>
> On OpenBSD, calloc(3) and reallocarray(3) check for integer overflow
> for you, which is why they are preferred over malloc(nmemb * size).
> You can examing the code yourself:
> http://cvsweb.openbsd.org/src/lib/libc/stdlib/reallocarray.c?rev=1.3
>
>  - todd
>
--
-Luke
Reply | Threaded
Open this post in threaded view
|

Re: if calloc() needs nmemb and size, why doesn't freezero()?

Luke Small
I used the verbiage: “malloc(3)” as a general all-encompassing manpage
which includes malloc(), calloc(), freezero(), etc.

Sorry for the confusion.

> In malloc(3):
>> > “If you use smaller integer types than size_t for ‘nmemb’ and ‘size’,
>> then
>> > multiplication in freezero() may need to be cast to size_t to avoid
>> integer
>> > overflow:
>> > freezero(ptr, (size_t)nmemb * (size_t)size);”
>> > Or maybe even: freezero(ptr, (size_t)nmemb * size);
>>
>> This is bad advice.  The product of two size_t values can exceed
>> SIZE_MAX, at which point you would get integer overflow.  This is
>> why the malloc(3) man page warns against it.  Note that on 64-bit
>> platforms like amd64, size_t is already 64-bit so casting to unsigned
>> long long or uint64_t is not effective.
>>
>> On OpenBSD, calloc(3) and reallocarray(3) check for integer overflow
>> for you, which is why they are preferred over malloc(nmemb * size).
>> You can examing the code yourself:
>> http://cvsweb.openbsd.org/src/lib/libc/stdlib/reallocarray.c?rev=1.3
>>
>>  - todd
>>
> --
> -Luke
>
--
-Luke