<div dir="ltr">Hello Maxim,<div><br></div><div>I changed the patch to check only the p->next pointer.</div><div>And checking if the page is in an address less than the (pool->pages + pages).</div><div><br></div><div>

<div>+    ngx_slab_page_t *prev, *p;</div><div>+    ngx_uint_t        pages;</div><div>+    size_t            size;</div><div>+</div><div>+    size = pool->end - (u_char *) pool - sizeof(ngx_slab_pool_t);</div><div>+    pages = (ngx_uint_t) (size / (ngx_pagesize + sizeof(ngx_slab_page_t)));</div>

<div>+</div><div>+    p = &page[page->slab];</div><div>+</div><div>+    if ((p < pool->pages + pages) &&</div><div>+            (p->next != NULL) &&</div><div>+            ((p->prev & NGX_SLAB_PAGE_MASK) == NGX_SLAB_PAGE)) {</div>

</div><div><br></div><div><br></div><div>I hope that now I did the right checks.</div><div><br></div><div>Regards,</div><div>Wandenberg</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Dec 20, 2013 at 2:49 PM, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello!<br>
<div class="im"><br>
On Tue, Dec 17, 2013 at 09:05:16PM -0200, Wandenberg Peixoto wrote:<br>
<br>
> Hi Maxim,<br>
><br>
> sorry for the long delay. I hope you remember my issue.<br>
> In attach is the new patch with the changes you suggest.<br>
> Can you check it again? I hope it can be applied to nginx code now.<br>
><br>
> About this point "2. There is probably no need to check both prev and<br>
> next.",<br>
> I check both pointers to avoid a segmentation fault, since in some<br>
> situations the next can be NULL and the prev may point to pool->free.<br>
> As far as I could follow the code seems to me that could happen one of this<br>
> pointers, next or prev, point to a NULL.<br>
> I just made a double check to protect.<br>
<br>
</div>As far as I see, all pages in the pool->free list are expected to<br>
have both p->prev and p->next set.  And all pages with type<br>
NGX_SLAB_PAGE will be either on the pool->free list, or will have<br>
p->next set to NULL.<br>
<br>
[...]<br>
<div class="im"><br>
> > > +{<br>
> > > +    ngx_slab_page_t *neighbour = &page[page->slab];<br>
> ><br>
> > Here "neighbour" may point outside of the allocated page<br>
> > structures if we are freeing the last page in the pool.<br>
<br>
</div>It looks like you've tried to address this problem with the<br>
following check:<br>
<br>
> +static ngx_int_t<br>
> +ngx_slab_merge_pages(ngx_slab_pool_t *pool, ngx_slab_page_t *page)<br>
> +{<br>
> +    ngx_slab_page_t *prev, *p;<br>
> +<br>
> +    p = &page[page->slab];<br>
> +    if ((u_char *) p >= pool->end) {<br>
> +        return NGX_DECLINED;<br>
> +    }<br>
<br>
This looks wrong, as pool->end points to the end of the pool, not<br>
the end of allocated page structures.<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Maxim Dounin<br>
<a href="http://nginx.org/" target="_blank">http://nginx.org/</a><br>
</font></span><div class="HOEnZb"><div class="h5"><br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</div></div></blockquote></div><br></div>