[PATCH] Use size_t for mallinfo fields.

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

[PATCH] Use size_t for mallinfo fields.

Martin Liška
Hi.

The current int type can easily overflow for allocation of more
than 4GB.

The following patch changes that to size_t. I guess I need to adjust
the API version of the function, right?

Thanks,
Martin

---
  malloc/malloc.h    | 20 ++++++++++----------
  manual/memory.texi | 22 +++++++++++-----------
  2 files changed, 21 insertions(+), 21 deletions(-)



0001-Use-size_t-for-mallinfo-fields.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Andreas Schwab-2
On Jul 07 2020, Martin Liška wrote:

> The current int type can easily overflow for allocation of more
> than 4GB.
>
> The following patch changes that to size_t. I guess I need to adjust
> the API version of the function, right?

Not only that, it breaks the ABI of mallinfo.

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
On 7/7/20 2:17 PM, Andreas Schwab wrote:
> On Jul 07 2020, Martin Liška wrote:
>
>> The current int type can easily overflow for allocation of more
>> than 4GB.
>>
>> The following patch changes that to size_t. I guess I need to adjust
>> the API version of the function, right?
>
> Not only that, it breaks the ABI of mallinfo.

Sure, so what options do I have? I'm new to glibc so a hint would be appreciated.

Thanks,
Martin

>
> Andreas.
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Sourceware - libc-alpha mailing list
On Tue, Jul 7, 2020 at 6:08 AM Martin Liška <[hidden email]> wrote:

>
> On 7/7/20 2:17 PM, Andreas Schwab wrote:
> > On Jul 07 2020, Martin Liška wrote:
> >
> >> The current int type can easily overflow for allocation of more
> >> than 4GB.
> >>
> >> The following patch changes that to size_t. I guess I need to adjust
> >> the API version of the function, right?
> >
> > Not only that, it breaks the ABI of mallinfo.
>
> Sure, so what options do I have? I'm new to glibc so a hint would be appreciated.
>

You need to keep the old version of mallinfo in libc.so.  There are
plenty of examples
in glibc source:

malloc/hooks.c:#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_25)
malloc/hooks.c:#endif   /* SHLIB_COMPAT */
malloc/malloc.c:#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_24)
malloc/malloc.c:#if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_26)
malloc/obstack.c:#  if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_3_4)

--
H.J.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Sourceware - libc-alpha mailing list
In reply to this post by Martin Liška
* Martin Liška:

> On 7/7/20 2:17 PM, Andreas Schwab wrote:
>> On Jul 07 2020, Martin Liška wrote:
>>
>>> The current int type can easily overflow for allocation of more
>>> than 4GB.
>>>
>>> The following patch changes that to size_t. I guess I need to adjust
>>> the API version of the function, right?
>>
>> Not only that, it breaks the ABI of mallinfo.
>
> Sure, so what options do I have? I'm new to glibc so a hint would be
> appreciated.

We need to add a new function.  Symbol versioning does not work because
mallinfo is interposed by alternative mallocs (tcmalloc, Address
Sanitizer, etc.).  Without the new function name, the interposer does
not know which ABI the application expects, so it's going to be quite
messy.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
On 7/7/20 3:49 PM, Florian Weimer wrote:

> * Martin Liška:
>
>> On 7/7/20 2:17 PM, Andreas Schwab wrote:
>>> On Jul 07 2020, Martin Liška wrote:
>>>
>>>> The current int type can easily overflow for allocation of more
>>>> than 4GB.
>>>>
>>>> The following patch changes that to size_t. I guess I need to adjust
>>>> the API version of the function, right?
>>>
>>> Not only that, it breaks the ABI of mallinfo.
>>
>> Sure, so what options do I have? I'm new to glibc so a hint would be
>> appreciated.
>
> We need to add a new function.  Symbol versioning does not work because
> mallinfo is interposed by alternative mallocs (tcmalloc, Address
> Sanitizer, etc.).  Without the new function name, the interposer does
> not know which ABI the application expects, so it's going to be quite
> messy.
>
> Thanks,
> Florian
>
All right, am I closer with the suggested patch?

Martin

0001-Use-size_t-for-mallinfo-fields.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Sourceware - libc-alpha mailing list
* Martin Liška:

> On 7/7/20 3:49 PM, Florian Weimer wrote:
>> * Martin Liška:
>>
>>> On 7/7/20 2:17 PM, Andreas Schwab wrote:
>>>> On Jul 07 2020, Martin Liška wrote:
>>>>
>>>>> The current int type can easily overflow for allocation of more
>>>>> than 4GB.
>>>>>
>>>>> The following patch changes that to size_t. I guess I need to adjust
>>>>> the API version of the function, right?
>>>>
>>>> Not only that, it breaks the ABI of mallinfo.
>>>
>>> Sure, so what options do I have? I'm new to glibc so a hint would be
>>> appreciated.
>>
>> We need to add a new function.  Symbol versioning does not work because
>> mallinfo is interposed by alternative mallocs (tcmalloc, Address
>> Sanitizer, etc.).  Without the new function name, the interposer does
>> not know which ABI the application expects, so it's going to be quite
>> messy.

> All right, am I closer with the suggested patch?

If what I wrote above is right (we'd first gather consensus around
that), we should probably add struct mallinfo2 and mallinfo2, deprecate
the original mallinfo function, and eventually remove them from the
public API (turning the original mallinfo into a compatibility symbol).

I suppose it would make sense to raise this issue with the tcmalloc, tbb
and Address Sanitizer people, to see if they would be willing to
implement mallinfo2 on their end.

The end result, having mallinfo2 and not mallinfo, is a bit ugly, but
it's an improvement over the current state.  I do not see a need to get
creative with symbol redirects or symbol versions.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Andreas Schwab-2
On Jul 07 2020, Florian Weimer wrote:

> If what I wrote above is right (we'd first gather consensus around
> that), we should probably add struct mallinfo2 and mallinfo2, deprecate
> the original mallinfo function, and eventually remove them from the
> public API (turning the original mallinfo into a compatibility symbol).

Isn't mallinfo obsoleted by malloc_info anyway?

Andreas.

--
Andreas Schwab, [hidden email]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Sourceware - libc-alpha mailing list
* Andreas Schwab:

> On Jul 07 2020, Florian Weimer wrote:
>
>> If what I wrote above is right (we'd first gather consensus around
>> that), we should probably add struct mallinfo2 and mallinfo2, deprecate
>> the original mallinfo function, and eventually remove them from the
>> public API (turning the original mallinfo into a compatibility symbol).
>
> Isn't mallinfo obsoleted by malloc_info anyway?

None of the malloc alternatives seem to have picked it up.

Martin, why do you want to change mallinfo, rather than switching to
malloc_info?

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
In reply to this post by Sourceware - libc-alpha mailing list
On 7/7/20 4:22 PM, Florian Weimer wrote:

> * Martin Liška:
>
>> On 7/7/20 3:49 PM, Florian Weimer wrote:
>>> * Martin Liška:
>>>
>>>> On 7/7/20 2:17 PM, Andreas Schwab wrote:
>>>>> On Jul 07 2020, Martin Liška wrote:
>>>>>
>>>>>> The current int type can easily overflow for allocation of more
>>>>>> than 4GB.
>>>>>>
>>>>>> The following patch changes that to size_t. I guess I need to adjust
>>>>>> the API version of the function, right?
>>>>>
>>>>> Not only that, it breaks the ABI of mallinfo.
>>>>
>>>> Sure, so what options do I have? I'm new to glibc so a hint would be
>>>> appreciated.
>>>
>>> We need to add a new function.  Symbol versioning does not work because
>>> mallinfo is interposed by alternative mallocs (tcmalloc, Address
>>> Sanitizer, etc.).  Without the new function name, the interposer does
>>> not know which ABI the application expects, so it's going to be quite
>>> messy.
>
>> All right, am I closer with the suggested patch?
>
> If what I wrote above is right (we'd first gather consensus around
> that), we should probably add struct mallinfo2 and mallinfo2, deprecate
> the original mallinfo function, and eventually remove them from the
> public API (turning the original mallinfo into a compatibility symbol).
All right, I'm sending patch for that.

>
> I suppose it would make sense to raise this issue with the tcmalloc, tbb
> and Address Sanitizer people, to see if they would be willing to
> implement mallinfo2 on their end.

Once we're done I can file issues to all these to inform them.

Thoughts?
Martin

>
> The end result, having mallinfo2 and not mallinfo, is a bit ugly, but
> it's an improvement over the current state.  I do not see a need to get
> creative with symbol redirects or symbol versions.
>
> Thanks,
> Florian
>


0001-Add-mallinfo2-function-that-support-sizes-4GB.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
In reply to this post by Sourceware - libc-alpha mailing list
On 7/7/20 4:36 PM, Florian Weimer wrote:

> * Andreas Schwab:
>
>> On Jul 07 2020, Florian Weimer wrote:
>>
>>> If what I wrote above is right (we'd first gather consensus around
>>> that), we should probably add struct mallinfo2 and mallinfo2, deprecate
>>> the original mallinfo function, and eventually remove them from the
>>> public API (turning the original mallinfo into a compatibility symbol).
>>
>> Isn't mallinfo obsoleted by malloc_info anyway?
>
> None of the malloc alternatives seem to have picked it up.
>
> Martin, why do you want to change mallinfo, rather than switching to
> malloc_info?

We use it in the GCC to inform about current memory usage (it's handy for LTO debugging).

If I see correctly, for malloc_info, one would have to parse a XML output..

Martin

>
> Thanks,
> Florian
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
In reply to this post by Martin Liška
PING^1

On 7/8/20 9:24 AM, Martin Liška wrote:

> On 7/7/20 4:22 PM, Florian Weimer wrote:
>> * Martin Liška:
>>
>>> On 7/7/20 3:49 PM, Florian Weimer wrote:
>>>> * Martin Liška:
>>>>
>>>>> On 7/7/20 2:17 PM, Andreas Schwab wrote:
>>>>>> On Jul 07 2020, Martin Liška wrote:
>>>>>>
>>>>>>> The current int type can easily overflow for allocation of more
>>>>>>> than 4GB.
>>>>>>>
>>>>>>> The following patch changes that to size_t. I guess I need to adjust
>>>>>>> the API version of the function, right?
>>>>>>
>>>>>> Not only that, it breaks the ABI of mallinfo.
>>>>>
>>>>> Sure, so what options do I have? I'm new to glibc so a hint would be
>>>>> appreciated.
>>>>
>>>> We need to add a new function.  Symbol versioning does not work because
>>>> mallinfo is interposed by alternative mallocs (tcmalloc, Address
>>>> Sanitizer, etc.).  Without the new function name, the interposer does
>>>> not know which ABI the application expects, so it's going to be quite
>>>> messy.
>>
>>> All right, am I closer with the suggested patch?
>>
>> If what I wrote above is right (we'd first gather consensus around
>> that), we should probably add struct mallinfo2 and mallinfo2, deprecate
>> the original mallinfo function, and eventually remove them from the
>> public API (turning the original mallinfo into a compatibility symbol).
>
> All right, I'm sending patch for that.
>
>>
>> I suppose it would make sense to raise this issue with the tcmalloc, tbb
>> and Address Sanitizer people, to see if they would be willing to
>> implement mallinfo2 on their end.
>
> Once we're done I can file issues to all these to inform them.
>
> Thoughts?
> Martin
>
>>
>> The end result, having mallinfo2 and not mallinfo, is a bit ugly, but
>> it's an improvement over the current state.  I do not see a need to get
>> creative with symbol redirects or symbol versions.
>>
>> Thanks,
>> Florian
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Szabolcs Nagy-2
In reply to this post by Martin Liška
The 07/08/2020 09:24, Martin Liška wrote:
> Subject: [PATCH] Add mallinfo2 function that support sizes >= 4GB.
>
> The current int type can easily overflow for allocation of more
> than 4GB.

i don't think a new symbol with a similar bad
design as the old one is desirable.

(i think a good design would allow exposing malloc
internal info without abi and api issues when the
internals change, e.g. no struct field names and
struct layout that are tied to internals. and
supportable by other implementations so whatever
gcc is doing would work elsewhere not just on glibc)

one hack that may be acceptable is to use some
nonsensical value in a mallinfo field to signal that
the fields have new meaning, then the api can work
backward compatibly when all field values are less
than 2G and after that things are broken anyway so
we can switch to some different struct content that
has no overflow issue, but abi compatible with the
old struct (e.g. round up the values to multiples of
1M), so we don't need a new abi symbol and interposers
continue to work. (but i'm not entirely sure this
is a good idea either)
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
On 7/23/20 4:38 PM, Szabolcs Nagy wrote:

> The 07/08/2020 09:24, Martin Liška wrote:
>> Subject: [PATCH] Add mallinfo2 function that support sizes >= 4GB.
>>
>> The current int type can easily overflow for allocation of more
>> than 4GB.
>
> i don't think a new symbol with a similar bad
> design as the old one is desirable.
>
> (i think a good design would allow exposing malloc
> internal info without abi and api issues when the
> internals change, e.g. no struct field names and
> struct layout that are tied to internals. and
> supportable by other implementations so whatever
> gcc is doing would work elsewhere not just on glibc)

Hello.

All right, I agree with that. So something like:

enum malloc_info
{
   ARENA_BYTES,
   ...
};

size_t get_mallinfo (malloc_info type) ?

That would allow adding new enum values that can supported in the future.

>
> one hack that may be acceptable is to use some
> nonsensical value in a mallinfo field to signal that
> the fields have new meaning, then the api can work
> backward compatibly when all field values are less
> than 2G and after that things are broken anyway so
> we can switch to some different struct content that
> has no overflow issue, but abi compatible with the
> old struct (e.g. round up the values to multiples of
> 1M), so we don't need a new abi symbol and interposers
> continue to work. (but i'm not entirely sure this
> is a good idea either)

Huh, that's quite hacky approach and I don't like it much :)
Let's forget about the old API/ABI and design a new proper one.

Martin

>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Sourceware - libc-alpha mailing list
* Martin Liška:

> All right, I agree with that. So something like:
>
> enum malloc_info
> {
>   ARENA_BYTES,
>   ...
> };
>
> size_t get_mallinfo (malloc_info type) ?
>
> That would allow adding new enum values that can supported in the future.

It does not allow to obtain a consistent snapshot of multiple values,
though.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
On 7/27/20 2:21 PM, Florian Weimer wrote:

> * Martin Liška:
>
>> All right, I agree with that. So something like:
>>
>> enum malloc_info
>> {
>>    ARENA_BYTES,
>>    ...
>> };
>>
>> size_t get_mallinfo (malloc_info type) ?
>>
>> That would allow adding new enum values that can supported in the future.
>
> It does not allow to obtain a consistent snapshot of multiple values,
> though.

Good point!

So are we back to mallinfo2 which I suggested in patch?
Can you please make an opinion about it?

Thanks,
Martin

>
> Thanks,
> Florian
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
PING^1

On 7/27/20 2:45 PM, Martin Liška wrote:

> On 7/27/20 2:21 PM, Florian Weimer wrote:
>> * Martin Liška:
>>
>>> All right, I agree with that. So something like:
>>>
>>> enum malloc_info
>>> {
>>>    ARENA_BYTES,
>>>    ...
>>> };
>>>
>>> size_t get_mallinfo (malloc_info type) ?
>>>
>>> That would allow adding new enum values that can supported in the future.
>>
>> It does not allow to obtain a consistent snapshot of multiple values,
>> though.
>
> Good point!
>
> So are we back to mallinfo2 which I suggested in patch?
> Can you please make an opinion about it?
>
> Thanks,
> Martin
>
>>
>> Thanks,
>> Florian
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Florian Weimer
In reply to this post by Martin Liška
* Martin Liška:

> On 7/27/20 2:21 PM, Florian Weimer wrote:
>> * Martin Liška:
>>
>>> All right, I agree with that. So something like:
>>>
>>> enum malloc_info
>>> {
>>>    ARENA_BYTES,
>>>    ...
>>> };
>>>
>>> size_t get_mallinfo (malloc_info type) ?
>>>
>>> That would allow adding new enum values that can supported in the future.
>>
>> It does not allow to obtain a consistent snapshot of multiple values,
>> though.
>
> Good point!
>
> So are we back to mallinfo2 which I suggested in patch?
> Can you please make an opinion about it?

DJ, what do you think about this patch?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Sourceware - libc-alpha mailing list
Florian Weimer <[hidden email]> writes:
> DJ, what do you think about this patch?

I have no real problems with the patch, but two minor things that could
be handled in a follow-up patch...

1. The copy code for the old function doesn't handle overflow.  We've
   seen bug reports for this before so should consider the edge cases.
   IMHO if a size_t value is larger than MAXINT, then MAXINT (or -1)
   should be stored instead of a randomly truncated value.

2. The new documentation makes no mention of the older "compatible"
   interface.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Use size_t for mallinfo fields.

Martin Liška
On 8/11/20 7:08 PM, DJ Delorie wrote:
> Florian Weimer <[hidden email]> writes:
>> DJ, what do you think about this patch?
>
> I have no real problems with the patch, but two minor things that could
> be handled in a follow-up patch...

Thank you for the review.
Can I read it as ready to go into master?

>
> 1. The copy code for the old function doesn't handle overflow.  We've
>     seen bug reports for this before so should consider the edge cases.
>     IMHO if a size_t value is larger than MAXINT, then MAXINT (or -1)
>     should be stored instead of a randomly truncated value.
>
> 2. The new documentation makes no mention of the older "compatible"
>     interface.
>

Both comments are valid to me and I can address them in a follow-up patch.

Martin
12