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
