Hi Maxim,
Any chance you can fix up this patch and get it into 1.7?
This patch is not compatible with 1.6.
Best,
John
On Wed, Jan 22, 2014 at 8:51 AM, Maxim Dounin <mdou...@mdounin.ru> wrote:
> Hello!
>
> On Wed, Jan 22, 2014 at 01:39:54AM -0200, Wandenberg Peixoto wrote:
>
>> Hello Maxim,
>>
>> did you have opportunity to take a look on this last patch?
>
> It looks more or less correct, though I don't happy with the
> checks done, and there are various style issues. I'm planning to
> look into it and build a better version as time permits.
>
>>
>>
>> Regards,
>> Wandenberg
>>
>>
>> On Thu, Dec 26, 2013 at 10:12 PM, Wandenberg Peixoto
>> <wandenb...@gmail.com>wrote:
>>
>> > Hello Maxim,
>> >
>> > I changed the patch to check only the p->next pointer.
>> > And checking if the page is in an address less than the (pool->pages +
>> > pages).
>> >
>> > + ngx_slab_page_t *prev, *p;
>> > + ngx_uint_t pages;
>> > + size_t size;
>> > +
>> > + size = pool->end - (u_char *) pool - sizeof(ngx_slab_pool_t);
>> > + pages = (ngx_uint_t) (size / (ngx_pagesize +
>> > sizeof(ngx_slab_page_t)));
>> > +
>> > + p = &page[page->slab];
>> > +
>> > + if ((p < pool->pages + pages) &&
>> > + (p->next != NULL) &&
>> > + ((p->prev & NGX_SLAB_PAGE_MASK) == NGX_SLAB_PAGE)) {
>> >
>> >
>> > I hope that now I did the right checks.
>> >
>> > Regards,
>> > Wandenberg
>> >
>> >
>> > On Fri, Dec 20, 2013 at 2:49 PM, Maxim Dounin <mdou...@mdounin.ru> wrote:
>> >
>> >> Hello!
>> >>
>> >> On Tue, Dec 17, 2013 at 09:05:16PM -0200, Wandenberg Peixoto wrote:
>> >>
>> >> > Hi Maxim,
>> >> >
>> >> > sorry for the long delay. I hope you remember my issue.
>> >> > In attach is the new patch with the changes you suggest.
>> >> > Can you check it again? I hope it can be applied to nginx code now.
>> >> >
>> >> > About this point "2. There is probably no need to check both prev and
>> >> > next.",
>> >> > I check both pointers to avoid a segmentation fault, since in some
>> >> > situations the next can be NULL and the prev may point to pool->free.
>> >> > As far as I could follow the code seems to me that could happen one of
>> >> this
>> >> > pointers, next or prev, point to a NULL.
>> >> > I just made a double check to protect.
>> >>
>> >> As far as I see, all pages in the pool->free list are expected to
>> >> have both p->prev and p->next set. And all pages with type
>> >> NGX_SLAB_PAGE will be either on the pool->free list, or will have
>> >> p->next set to NULL.
>> >>
>> >> [...]
>> >>
>> >> > > > +{
>> >> > > > + ngx_slab_page_t *neighbour = &page[page->slab];
>> >> > >
>> >> > > Here "neighbour" may point outside of the allocated page
>> >> > > structures if we are freeing the last page in the pool.
>> >>
>> >> It looks like you've tried to address this problem with the
>> >> following check:
>> >>
>> >> > +static ngx_int_t
>> >> > +ngx_slab_merge_pages(ngx_slab_pool_t *pool, ngx_slab_page_t *page)
>> >> > +{
>> >> > + ngx_slab_page_t *prev, *p;
>> >> > +
>> >> > + p = &page[page->slab];
>> >> > + if ((u_char *) p >= pool->end) {
>> >> > + return NGX_DECLINED;
>> >> > + }
>> >>
>> >> This looks wrong, as pool->end points to the end of the pool, not
>> >> the end of allocated page structures.
>> >>
>> >> --
>> >> Maxim Dounin
>> >> http://nginx.org/
>> >>
>> >> _______________________________________________
>> >> nginx-devel mailing list
>> >> nginx-devel@nginx.org
>> >> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>> >>
>> >
>> >
>
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel@nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
>
> --
> Maxim Dounin
> http://nginx.org/
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
