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 <mdounin@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