<div dir="ltr">Thanks Maxim. I did not notice that check of a->elts. <div>I understand to not to fix the memory optimization as you do not want to bring in pool intelligence into array utility. Still, I think we used pool data to some extend. If there is advantage with memory optimization I think we should think about handling it.</div>
<div><br></div><div>Thanks,</div><div>-Ravi.<br><div><br></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 16, 2014 at 8:04 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 Thu, Jan 16, 2014 at 07:16:50PM -0800, Ravi Chunduru wrote:<br>
<br>
> Hi Maxim,<br>
> I understand on array overflow nginx creates a new memory block. That is<br>
> perfectly alright.<br>
><br>
> Say I have a Pool and array is allocated from the first memory block and it<br>
> happend such a way that array elements end at pool->d.last.<br>
><br>
> Now, say the pool is used for some other purposes such a way that<br>
> pool->current is now pointing to a different memory block say '2'<br>
><br>
> And if we want to allocate a few more array elements, nginx has to use<br>
> second memory block. Now the elements are moved to second memory block.<br>
><br>
> At this stage, if any new element is requested that results in overflow,<br>
> nginx does the below checks<br>
><br>
> if ((u_char *) a->elts + size == p->d.last<br>
><br>
> && p->d.last + a->size <= p->d.end)<br>
><br>
> In the above code, p->d.last was actually pointing to the end of first<br>
</div>> memory block but not second memory block. Hence *even though there is<br>
> memory available in second memory block* it will go ahead and create a new<br>
<div class="im">> memory block. This will repeat on each overflow.<br>
<br>
</div>Yes, I understand what you are talking about. As array never<br>
knows from which exactly block the memory was allocated, and<br>
doesn't want to dig into pool internals, it only checks an obvious<br>
case - if the memory was allocated from the first block, and if<br>
there is a room there.<br>
<div class="im"><br>
> And, the code in ngx_array_destroy() will actually set the<br>
> pointer(p->d.last) wrongly once there is a overflow. This is a critical<br>
> issue.<br>
> First memory block would have say 'n' elements but after overflow number of<br>
> elements become 2 times of n.<br>
> Lets say after second overflow, I destroyed the array, then p->d.last will<br>
> be set backwards by 2 times in the first memory block. But, in actuality it<br>
> was size 'n'.<br>
<br>
</div>The code in ngx_array_destroy() does the following:<br>
<br>
if ((u_char *) a->elts + a->size * a->nalloc == p->d.last) {<br>
p->d.last -= a->size * a->nalloc;<br>
}<br>
<br>
If a memory was allocated from another block, the "a->elts + ...<br>
== p->d.last" check will fail and p->d.last will not be moved.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> Nginx never faces that situation because, once a memory block is set to<br>
> 'failed', it wont be used for allocation any more. But, if the 'failed'<br>
> count is less than 4 then we may have issue and also pool destroy may have<br>
> potential issues.<br>
><br>
> Sorry for long email, but I wanted to explain that in detail.<br>
><br>
> Thanks,<br>
> -Ravi.<br>
><br>
><br>
><br>
> On Thu, Jan 16, 2014 at 6:40 PM, Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>> wrote:<br>
><br>
> > Hello!<br>
> ><br>
> > On Thu, Jan 16, 2014 at 06:22:58PM -0800, Ravi Chunduru wrote:<br>
> ><br>
> > > Hi Nginx experts,<br>
> > > Thanks for the prompt reply to my earlier email on ngx_reset_pool()<br>
> > ><br>
> > > Now, I am looking into ngx_array.c. I found an issue ngx_array_push().<br>
> > Here<br>
> > > are the details.<br>
> > > nginx will check if number of elements is equal to capacity of the array.<br>
> > > If there is no space in the memory block, it allocates a new memory block<br>
> > > with twice the size of array and copies over the elements. So far so<br>
> > good.<br>
> > > Assume that pool utility got entirely new memory block then a->pool is<br>
> > not<br>
> > > updated with that of 'pool->current'.<br>
> > ><br>
> > > I got an assumption from the code that a->pool is always the memory block<br>
> > > that has the array elements by seeing the code in ngx_array_push(),<br>
> > > ngx_array_push_n() or ngx_array_destroy() where checks were always done<br>
> > > with pool pointer in array.<br>
> > ><br>
> > > Functionalities issues would come up once there is an array overflow. I<br>
> > > think for every new push of element after first crossing/overflow of the<br>
> > > capacity, nginx will keep on creating new array. Thus it results in<br>
> > wastage<br>
> > > of memory.<br>
> > ><br>
> > > Please let me know if its a issue or correct my understanding.<br>
> ><br>
> > That's expected behaviour. Arrays are implemented in a way that<br>
> > allocates additional memory on overflows, and it's expected to<br>
> > happen. There is a ngx_list_t structure to be used if such<br>
> > additional memory allocations are undesired. Optimization of<br>
> > allocations which uses pool internals is just an optimization and<br>
> > it's not expected to always succeed.<br>
> ><br>
> > --<br>
> > Maxim Dounin<br>
> > <a href="http://nginx.org/" target="_blank">http://nginx.org/</a><br>
> ><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>
> ><br>
><br>
><br>
><br>
> --<br>
> Ravi<br>
<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>
<br>
<br>
--<br>
Maxim Dounin<br>
<a href="http://nginx.org/" target="_blank">http://nginx.org/</a><br>
<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><br clear="all"><div><br></div>-- <br>Ravi<br>
</div>