Re: Segfaults in ConnectionHander (Possible Solution)

[Graham Dumpleton <gr...@dscpl.com.au>]

Graham Dumpleton wrote ..
> Returning back up to _conn_read() in mod_python source code, we have
> where core_input_filter() was called ap_get_brigade():
> 
>     Py_BEGIN_ALLOW_THREADS;
>     rc = ap_get_brigade(c->input_filters, bb, mode, APR_BLOCK_READ, bufsize);
>     Py_END_ALLOW_THREADS;
>     
>     if (! APR_STATUS_IS_SUCCESS(rc)) {
>         PyErr_SetObject(PyExc_IOError, 
>                         PyString_FromString("Connection read error"));
>         return NULL;
>     }
> 
> Since APR_SUCCESS was returned and assigned to "rc", no problem is detected.
> 
> The code which follows then assumes that the first bucket in the bucket
> brigade actually contains valid data, when in fact the first bucket is
> actually
> crap as nothing was done to set up a valid bucket since EAGAIN was returned.
> As a consequence it crashes.
> 
> Thus in summary, _conn_read() doesn't cater in any way for the possibility
> that the initial socket read may have failed because of EAGAIN and thus
> the bucket is bogus. The problem is, how is it mean't to know this if the
> value APR_SUCCESS is returned by ap_get_brigade().

Extending the above code as:

    Py_BEGIN_ALLOW_THREADS;
    rc = ap_get_brigade(c->input_filters, bb, mode, APR_BLOCK_READ, bufsize);
    Py_END_ALLOW_THREADS;

    if (! APR_STATUS_IS_SUCCESS(rc)) {
        PyErr_SetObject(PyExc_IOError,
                        PyString_FromString("Connection read error"));
        return NULL;
    }

    /* Return empty string if no buckets. Can be caused by EAGAIN. */
    if (APR_BRIGADE_EMPTY(bb)) {
        return PyString_FromString("");
    }

seems to fix the problem. Ie., use call to APR_BRIGADE_EMPTY(bb) to check
whether any new buckets added and returning empty string if not.

Can someone else seeing this issue try this fix and see if the tests then
work.

Graham

Re: Segfaults in ConnectionHander (Possible Solution)

[Jim Gallacher <jp...@jgassociates.ca>]

Graham Dumpleton wrote:
> Graham Dumpleton wrote ..
> 
>>Returning back up to _conn_read() in mod_python source code, we have
>>where core_input_filter() was called ap_get_brigade():
>>
>>    Py_BEGIN_ALLOW_THREADS;
>>    rc = ap_get_brigade(c->input_filters, bb, mode, APR_BLOCK_READ, bufsize);
>>    Py_END_ALLOW_THREADS;
>>    
>>    if (! APR_STATUS_IS_SUCCESS(rc)) {
>>        PyErr_SetObject(PyExc_IOError, 
>>                        PyString_FromString("Connection read error"));
>>        return NULL;
>>    }
>>
>>Since APR_SUCCESS was returned and assigned to "rc", no problem is detected.
>>
>>The code which follows then assumes that the first bucket in the bucket
>>brigade actually contains valid data, when in fact the first bucket is
>>actually
>>crap as nothing was done to set up a valid bucket since EAGAIN was returned.
>>As a consequence it crashes.
>>
>>Thus in summary, _conn_read() doesn't cater in any way for the possibility
>>that the initial socket read may have failed because of EAGAIN and thus
>>the bucket is bogus. The problem is, how is it mean't to know this if the
>>value APR_SUCCESS is returned by ap_get_brigade().
> 
> 
> Extending the above code as:
> 
>     Py_BEGIN_ALLOW_THREADS;
>     rc = ap_get_brigade(c->input_filters, bb, mode, APR_BLOCK_READ, bufsize);
>     Py_END_ALLOW_THREADS;
> 
>     if (! APR_STATUS_IS_SUCCESS(rc)) {
>         PyErr_SetObject(PyExc_IOError,
>                         PyString_FromString("Connection read error"));
>         return NULL;
>     }
> 
>     /* Return empty string if no buckets. Can be caused by EAGAIN. */
>     if (APR_BRIGADE_EMPTY(bb)) {
>         return PyString_FromString("");
>     }
> 
> seems to fix the problem. Ie., use call to APR_BRIGADE_EMPTY(bb) to check
> whether any new buckets added and returning empty string if not.
> 
> Can someone else seeing this issue try this fix and see if the tests then
> work.

Note that APR_STATUS_IS_SUCCESS has been removed from apr 1.x, which is 
one of the issues in getting mod_python to run in Apache 2.2. It looks 
like we should just check if rc != 0. This is according to discussion 
here featuring Greg Stein and Ryan Bloom:
http://www.mail-archive.com/dev@httpd.apache.org/msg21757.html

I'll update MODPYTHON-78 regarding Apache 2.2 with details on this and 
apr_sockaddr_port_get which has also been removed in apr 1.x.

Jim