serf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ivan Zhakov <i...@apache.org>
Subject Re: svn commit: r1712718 - /serf/trunk/buckets/allocator.c
Date Fri, 06 Nov 2015 07:12:06 GMT
On 6 November 2015 at 08:43, Greg Stein <gstein@gmail.com> wrote:
> On Thu, Nov 5, 2015 at 9:29 AM, Ivan Zhakov <ivan@apache.org> wrote:
>
>> > On Thu, Nov 5, 2015 at 1:40 AM, <ivan@apache.org> wrote:
>> >
>> >> Author: ivan
>> >> Date: Thu Nov  5 07:40:58 2015
>> >> New Revision: 1712718
>> >>
>> >> URL: http://svn.apache.org/viewvc?rev=1712718&view=rev
>> >> Log:
>> >> Add another diagnostic macro SERF__DEBUG_USE_AFTER_FREE to detect usage
>> of
>> >> bucket allocator after it destroyed by pool cleanup.
>> On 5 November 2015 at 16:31, Greg Stein <gstein@gmail.com> wrote:
>> > Seems this should be tried straight into the allocator debugging, rather
>> > than separately configurable. Use-after-free is quite bad, so I think it
>> > should always die right away.
>> >
>> Just to confirm that I'm understanding you properly: do you suggest to
>> enable this check by default and abort() on attempt to use bucket
>> allocator after it's destroyed?
>>
>
> It should never just abort() ...
I think aborting on use-after-free is acceptable behavior, since we
are pretty that program will crash anyway.

> That would be only when the debugging is
> turned on. I'm just saying it doesn't seem useful to have *another*
> debugging switch for USE_AFTER_FREE.
>
Currently we have DEBUG_DOUBLE_FREE which is *always* defined:
[[[
/* Define DEBUG_DOUBLE_FREE if you're interested in debugging double-free
 * calls to serf_bucket_mem_free().
 */
#define DEBUG_DOUBLE_FREE
]]]

Moving #ifdef SERF__DEBUG_USE_AFTER_FREE code to #ifdef
DEBUG_DOUBLE_FREE makes sense for me.

>> > Your work on reporting unfreed items is a bit different, as some
>> > applications *may* choose to leave some allocations until the process
>> exits.
>> >
>> I agree, but current documentation for serf_bucket_allocator_create()
>> says otherwise:
>>
>
> Oh. Heh. I think we should leave the doc as-is for now. We may implement it
> one day (tracking doesn't have to be expensive), though we'd probably not
> want to abort() out of the blue for older apps that weren't careful.
>


-- 
Ivan Zhakov

Mime
View raw message