You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2015/03/11 15:44:23 UTC
svn commit: r1665888 - /tomcat/native/branches/1.1.x/native/src/poll.c
Author: markt
Date: Wed Mar 11 14:44:23 2015
New Revision: 1665888
URL: http://svn.apache.org/r1665888
Log:
Fix 57653. Crash when multiple events for same socket are returned via separate apr_pollfd_t structures
Modified:
tomcat/native/branches/1.1.x/native/src/poll.c
Modified: tomcat/native/branches/1.1.x/native/src/poll.c
URL: http://svn.apache.org/viewvc/tomcat/native/branches/1.1.x/native/src/poll.c?rev=1665888&r1=1665887&r2=1665888&view=diff
==============================================================================
--- tomcat/native/branches/1.1.x/native/src/poll.c (original)
+++ tomcat/native/branches/1.1.x/native/src/poll.c Wed Mar 11 14:44:23 2015
@@ -360,7 +360,13 @@ TCN_IMPLEMENT_CALL(jint, Poll, poll)(TCN
tcn_socket_t *s = (tcn_socket_t *)fd->client_data;
p->set[i*2+0] = (jlong)(fd->rtnevents);
p->set[i*2+1] = P2J(s);
- if (remove) {
+ /* If a socket is registered for multiple events and the poller has
+ multiple events to return it may do as a single pair in this
+ array or as multiple pairs depending on implementation. On OSX at
+ least, multiple pairs have been observed. In this case do not try
+ and remove socket from the pollset for a second time else a crash
+ will result. */
+ if (remove && s->pe) {
apr_pollset_remove(p->pollset, fd);
APR_RING_REMOVE(s->pe, link);
APR_RING_INSERT_TAIL(&p->dead_ring, s->pe, tcn_pfde_t, link);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665888 - /tomcat/native/branches/1.1.x/native/src/poll.c
Posted by Mark Thomas <ma...@apache.org>.
On 17/03/2015 10:28, Mark Thomas wrote:
> On 16/03/2015 21:40, Konstantin Kolinko wrote:
<snip/>
>> Also, there is a question of porting this fix to Native trunk.
>
> I'll take a look.
Based on my understanding of the trunk code, this is not an issue in trunk.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665888 - /tomcat/native/branches/1.1.x/native/src/poll.c
Posted by Mark Thomas <ma...@apache.org>.
On 16/03/2015 21:40, Konstantin Kolinko wrote:
> 2015-03-11 17:44 GMT+03:00 <ma...@apache.org>:
>> Author: markt
>> Date: Wed Mar 11 14:44:23 2015
>> New Revision: 1665888
>>
>> URL: http://svn.apache.org/r1665888
>> Log:
>> Fix 57653. Crash when multiple events for same socket are returned via separate apr_pollfd_t structures
>>
>> Modified:
>> tomcat/native/branches/1.1.x/native/src/poll.c
>>
>> Modified: tomcat/native/branches/1.1.x/native/src/poll.c
>> URL: http://svn.apache.org/viewvc/tomcat/native/branches/1.1.x/native/src/poll.c?rev=1665888&r1=1665887&r2=1665888&view=diff
>> ==============================================================================
>> --- tomcat/native/branches/1.1.x/native/src/poll.c (original)
>> +++ tomcat/native/branches/1.1.x/native/src/poll.c Wed Mar 11 14:44:23 2015
>> @@ -360,7 +360,13 @@ TCN_IMPLEMENT_CALL(jint, Poll, poll)(TCN
>> tcn_socket_t *s = (tcn_socket_t *)fd->client_data;
>> p->set[i*2+0] = (jlong)(fd->rtnevents);
>> p->set[i*2+1] = P2J(s);
>> - if (remove) {
>> + /* If a socket is registered for multiple events and the poller has
>> + multiple events to return it may do as a single pair in this
>> + array or as multiple pairs depending on implementation. On OSX at
>> + least, multiple pairs have been observed. In this case do not try
>> + and remove socket from the pollset for a second time else a crash
>> + will result. */
>> + if (remove && s->pe) {
>> apr_pollset_remove(p->pollset, fd);
>> APR_RING_REMOVE(s->pe, link);
>> APR_RING_INSERT_TAIL(&p->dead_ring, s->pe, tcn_pfde_t, link);
>
> There shall be a separate nested "if (s->pe)" block instead of
> combined condition. This "if" is followed by an "else" branch, which
> is now executed in an unexpected way.
>
> Lines 357-356 before the changed code:
> if (!remove)
> now = apr_time_now();
>
> Line 379-385 the "else" branch:
> else {
> /* Update last active with the current time
> * after the poll call.
> */
> s->last_active = now;
> }
>
> Note how "now" is uninitialized when "remove" flag is false.
Thanks. Fixed.
> Also, there is a question of porting this fix to Native trunk.
I'll take a look.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665888 - /tomcat/native/branches/1.1.x/native/src/poll.c
Posted by Konstantin Kolinko <kn...@gmail.com>.
2015-03-11 17:44 GMT+03:00 <ma...@apache.org>:
> Author: markt
> Date: Wed Mar 11 14:44:23 2015
> New Revision: 1665888
>
> URL: http://svn.apache.org/r1665888
> Log:
> Fix 57653. Crash when multiple events for same socket are returned via separate apr_pollfd_t structures
>
> Modified:
> tomcat/native/branches/1.1.x/native/src/poll.c
>
> Modified: tomcat/native/branches/1.1.x/native/src/poll.c
> URL: http://svn.apache.org/viewvc/tomcat/native/branches/1.1.x/native/src/poll.c?rev=1665888&r1=1665887&r2=1665888&view=diff
> ==============================================================================
> --- tomcat/native/branches/1.1.x/native/src/poll.c (original)
> +++ tomcat/native/branches/1.1.x/native/src/poll.c Wed Mar 11 14:44:23 2015
> @@ -360,7 +360,13 @@ TCN_IMPLEMENT_CALL(jint, Poll, poll)(TCN
> tcn_socket_t *s = (tcn_socket_t *)fd->client_data;
> p->set[i*2+0] = (jlong)(fd->rtnevents);
> p->set[i*2+1] = P2J(s);
> - if (remove) {
> + /* If a socket is registered for multiple events and the poller has
> + multiple events to return it may do as a single pair in this
> + array or as multiple pairs depending on implementation. On OSX at
> + least, multiple pairs have been observed. In this case do not try
> + and remove socket from the pollset for a second time else a crash
> + will result. */
> + if (remove && s->pe) {
> apr_pollset_remove(p->pollset, fd);
> APR_RING_REMOVE(s->pe, link);
> APR_RING_INSERT_TAIL(&p->dead_ring, s->pe, tcn_pfde_t, link);
There shall be a separate nested "if (s->pe)" block instead of
combined condition. This "if" is followed by an "else" branch, which
is now executed in an unexpected way.
Lines 357-356 before the changed code:
if (!remove)
now = apr_time_now();
Line 379-385 the "else" branch:
else {
/* Update last active with the current time
* after the poll call.
*/
s->last_active = now;
}
Note how "now" is uninitialized when "remove" flag is false.
Also, there is a question of porting this fix to Native trunk.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665888 - /tomcat/native/branches/1.1.x/native/src/poll.c
Posted by Mark Thomas <ma...@apache.org>.
On 11/03/2015 18:13, Christopher Schultz wrote:
> Rainer,
>
> On 3/11/15 12:38 PM, Rainer Jung wrote:
>> Am 11.03.2015 um 15:45 schrieb Mark Thomas:
>>> On 11/03/2015 14:44, markt@apache.org wrote:
>>>> Author: markt
>>>> Date: Wed Mar 11 14:44:23 2015
>>>> New Revision: 1665888
>>>>
>>>> URL: http://svn.apache.org/r1665888
>>>> Log:
>>>> Fix 57653. Crash when multiple events for same socket are returned
>>>> via separate apr_pollfd_t structures
>>>
>>> Review appreciated from folks that understand C better than I do (i.e.
>>> pretty much anybody). Hopefully it is clear from the comment what I am
>>> trying to do here and why.
>>
>> I guess it is more of a question of knowing the pollset and RING magic -
>> which I don't really qualify for - but it looks good to me.
>
> +1
>
>> Even if for
>> the two fd with the same socket their client_data parts might not be
>> identical, they should point to the same (identical) socket structure,
>> so NULLing "pe" in the socket during the first fd occurence should work
>> as a decision point for the second occurence.
>
> +1
>
> Is it a problem to call apr_pollset_remove for that file descriptor if
> it's not actually in the pollset? That is, are remove operations
> idempotent? Would it be considered an error (at a higher level) to try
> to remove the same fd twice?
You get a return code indicating that the fd wasn't in the pollset.
>> The only other case would be some fd which has a NUL s->pe from the
>> beginning, but then the existing code would have also crashed though
>> only after calling apr_pollset_remove(). So I guess that case is not
>> possible.
>
> +1
>
> What you (Mark) have done is just avoid a seg fault when s->pe is NULL.
Which should never happen apart from in the case that we are trying to
handle.
> What you haven't done (which might not actually be necessary) is solve
> any issue where this method shouldn't be called in the first place (i.e.
> state management).
I don't believe that is an issue.
We could have tried to merge multiple returns for the one fd but that
would have been a much more complicated patch. I'd rather deal with that
in the Java code.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665888 -
/tomcat/native/branches/1.1.x/native/src/poll.c
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Rainer,
On 3/11/15 12:38 PM, Rainer Jung wrote:
> Am 11.03.2015 um 15:45 schrieb Mark Thomas:
>> On 11/03/2015 14:44, markt@apache.org wrote:
>>> Author: markt
>>> Date: Wed Mar 11 14:44:23 2015
>>> New Revision: 1665888
>>>
>>> URL: http://svn.apache.org/r1665888
>>> Log:
>>> Fix 57653. Crash when multiple events for same socket are returned
>>> via separate apr_pollfd_t structures
>>
>> Review appreciated from folks that understand C better than I do (i.e.
>> pretty much anybody). Hopefully it is clear from the comment what I am
>> trying to do here and why.
>
> I guess it is more of a question of knowing the pollset and RING magic -
> which I don't really qualify for - but it looks good to me.
+1
> Even if for
> the two fd with the same socket their client_data parts might not be
> identical, they should point to the same (identical) socket structure,
> so NULLing "pe" in the socket during the first fd occurence should work
> as a decision point for the second occurence.
+1
Is it a problem to call apr_pollset_remove for that file descriptor if
it's not actually in the pollset? That is, are remove operations
idempotent? Would it be considered an error (at a higher level) to try
to remove the same fd twice?
> The only other case would be some fd which has a NUL s->pe from the
> beginning, but then the existing code would have also crashed though
> only after calling apr_pollset_remove(). So I guess that case is not
> possible.
+1
What you (Mark) have done is just avoid a seg fault when s->pe is NULL.
What you haven't done (which might not actually be necessary) is solve
any issue where this method shouldn't be called in the first place (i.e.
state management).
-chris
Re: svn commit: r1665888 - /tomcat/native/branches/1.1.x/native/src/poll.c
Posted by Rainer Jung <ra...@kippdata.de>.
Am 11.03.2015 um 15:45 schrieb Mark Thomas:
> On 11/03/2015 14:44, markt@apache.org wrote:
>> Author: markt
>> Date: Wed Mar 11 14:44:23 2015
>> New Revision: 1665888
>>
>> URL: http://svn.apache.org/r1665888
>> Log:
>> Fix 57653. Crash when multiple events for same socket are returned via separate apr_pollfd_t structures
>
> Review appreciated from folks that understand C better than I do (i.e.
> pretty much anybody). Hopefully it is clear from the comment what I am
> trying to do here and why.
I guess it is more of a question of knowing the pollset and RING magic -
which I don't really qualify for - but it looks good to me. Even if for
the two fd with the same socket their client_data parts might not be
identical, they should point to the same (identical) socket structure,
so NULLing "pe" in the socket during the first fd occurence should work
as a decision point for the second occurence.
The only other case would be some fd which has a NUL s->pe from the
beginning, but then the existing code would have also crashed though
only after calling apr_pollset_remove(). So I guess that case is not
possible.
Nice finding.
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1665888 - /tomcat/native/branches/1.1.x/native/src/poll.c
Posted by Mark Thomas <ma...@apache.org>.
On 11/03/2015 14:44, markt@apache.org wrote:
> Author: markt
> Date: Wed Mar 11 14:44:23 2015
> New Revision: 1665888
>
> URL: http://svn.apache.org/r1665888
> Log:
> Fix 57653. Crash when multiple events for same socket are returned via separate apr_pollfd_t structures
Review appreciated from folks that understand C better than I do (i.e.
pretty much anybody). Hopefully it is clear from the comment what I am
trying to do here and why.
Cheers,
Mark
> Modified:
> tomcat/native/branches/1.1.x/native/src/poll.c
>
> Modified: tomcat/native/branches/1.1.x/native/src/poll.c
> URL: http://svn.apache.org/viewvc/tomcat/native/branches/1.1.x/native/src/poll.c?rev=1665888&r1=1665887&r2=1665888&view=diff
> ==============================================================================
> --- tomcat/native/branches/1.1.x/native/src/poll.c (original)
> +++ tomcat/native/branches/1.1.x/native/src/poll.c Wed Mar 11 14:44:23 2015
> @@ -360,7 +360,13 @@ TCN_IMPLEMENT_CALL(jint, Poll, poll)(TCN
> tcn_socket_t *s = (tcn_socket_t *)fd->client_data;
> p->set[i*2+0] = (jlong)(fd->rtnevents);
> p->set[i*2+1] = P2J(s);
> - if (remove) {
> + /* If a socket is registered for multiple events and the poller has
> + multiple events to return it may do as a single pair in this
> + array or as multiple pairs depending on implementation. On OSX at
> + least, multiple pairs have been observed. In this case do not try
> + and remove socket from the pollset for a second time else a crash
> + will result. */
> + if (remove && s->pe) {
> apr_pollset_remove(p->pollset, fd);
> APR_RING_REMOVE(s->pe, link);
> APR_RING_INSERT_TAIL(&p->dead_ring, s->pe, tcn_pfde_t, link);
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org