On 8/2/22 3:19 PM, Yann Ylavic wrote:
> On Tue, Aug 2, 2022 at 2:59 PM Ruediger Pluem <rpl...@apache.org> wrote:
>>
>> On 8/2/22 2:31 PM, Yann Ylavic wrote:
>>> On Mon, Aug 1, 2022 at 8:15 PM Ruediger Pluem <rpl...@apache.org> wrote:
>>>>
>>>> On 8/1/22 5:05 PM, Ivan Zhakov wrote:
>>>>>
>>>>> My overall concern is that with the current solution as of r1902858,
>>>>> there are valid and
>>>>> reasonable allocation patterns that will cause an unbounded memory usage.
>>>>>
>>>>> For example, nothing prevents current or future PCRE versions from doing
>>>>> this:
>>>>>
>>>>> for (int i = 0; i < N; i++)
>>>>> {
>>>>> void *p = private_malloc();
>>>>> private_free(p);
>>>>> }
>>>>>
>>>>> which is going to cause an unbounded memory usage because private_free()
>>>>> is
>>>>> a no-op and retains everything that was allocated.
>>>>
>>>> This is true, but these kind of allocation patterns would also cause a lot
>>>> of problems with a malloc/free backend and thus I think
>>>> they are unlikely and would need to be addressed within PCRE. But even if
>>>> they are happening and cannot be fixed for our users
>>>> by adjusting PCRE e.g. because they are stuck to distribution versions we
>>>> still could tackle this then with an apr_bucket_alloc /
>>>> apr_bucket_free approach or worst case even with a malloc / free approach
>>>> for particular "bad" PCRE versions. But for now the
>>>> current code seems to work well with the current PCRE allocation pattern.
>>>
>>> We could switch to an apr_allocator+apr_memnode_t pattern just now,
>>> and trade the 8K used by the current (sub)pool for an overhead of
>>> sizeof(apr_memnode_t)+8 only, and private_free() just calls
>>> apr_allocator_free().
>>>
>>> Something like the attached patch?
>>
>> But this would allocate at least 8k for each private_malloc call, correct?
>
> Yes, 8K with apr-1.x (4K with apr-trunk IIRC).
>
> But anyway pcre2_match() is going to ask at least 40K for its first
> allocation (or 20K with next versions), because it doubles the
> number/size of the heap frames when needed and starts with 20K (either
> on the stack for the current version, or on the heap for the next
> versions [0]).
Thanks for the details as I was not aware of these, but this is what it
currently does and in the end the discussion from my point
of view was that it could do something entirely different. My point is more:
The current implementation with the sub pool is fine
with the current allocation pattern of PCRE. If the pattern changes we would
have ways to adopt the code on our side to react on
these changes.
Regards
Rüdiger
