wrong handler supplied to req.add_handler() generates error

• Key: MODPYTHON-98
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Graham Dumpleton
• Component/s: core
• Affects Version/s: 3.1.4, 3.2.7
• Fix Version/s: 3.2.7

Description

The documentation for req.add_handler() states:

  Note: There is no checking being done on the validity of the handler name. If you pass this function an invalid handler it will simply be ignored.

In other words, get the name of the handler wrong and it is supposed to just ignore it. This is not actually the case, instead it will generate an exception when it goes to process the handler:

  Mod_python error: "PythonHandler example::handler_3"

  Traceback (most recent call last):

    File "/System/Library/Frameworks/Python.framework/Versions/2.3/lib/python2.3/site-packages/mod_python/apache.py", line 291, in HandlerDispatch
      arg=req, silent=hlist.silent)

    File "/System/Library/Frameworks/Python.framework/Versions/2.3/lib/python2.3/site-packages/mod_python/apache.py", line 538, in resolve_object
      raise AttributeError, s

  AttributeError: module '/Users/grahamd/Sites/add_handler/example.py' contains no 'handler_3'

This can be seen with .htaccess file of:

  SetHandler mod_python
  PythonAccessHandler example
  PythonHandler example::handler_1
  PythonDebug On

and example.py file containing:

  from mod_python import apache

  def accesshandler(req):
    apache.log_error("accesshandler")
    req.add_handler("PythonHandler","example::handler_3")
    return apache.OK

  def handler_1(req):
    apache.log_error("handler_1")
    req.content_type = 'text/plain'
    req.write("HELLO")
    return apache.OK

  def handler_2(req):
    apache.log_error("handler_2")
    return apache.OK

Either the documentation is wrong and an exception is desired, or more likely this is an extension of the prior problem with hlist.silent as described as being a problem in other ways in MODPYTHON-46.

In that case the logic of SILENT/NOTSILENT was the wrong way around and it was fixed by reversing the definitions of the two. In doing this though, it didn't cover cases where a "silent" flag is passed to hlist_new() and hlist_append() in the req_add_handler() function of requestobject.c.

Specfically, there are calls to hlist_new() and hlist_append() in that function:

        hlist_append(self->request_rec->pool, self->hlo->head,
                     handler, dir, 0);

            hle = hlist_new(self->request_rec->pool, handler, dir, 0);

            hlist_append(self->request_rec->pool, hle, handler, dir, 0);

These should be written as:

        hlist_append(self->request_rec->pool, self->hlo->head,
                     handler, dir, SILENT);

            hle = hlist_new(self->request_rec->pool, handler, dir, SILENT);

            hlist_append(self->request_rec->pool, hle, handler, dir, SILENT);

If this change were made, the code would then behaves conformant with the documentation as far as being silent, however it highlights a further issue.

This further issue is that although it is silent when the handler name is wrong, this results in apache.DECLINED being returned for the handler that couldn't be found. Because apache.DECLINED is returned, Apache will try and interpret the URL again and if possible serve up a static file etc.

For the above example code this then means that if "example.py" was used in the URL, the browser gets back a response of:

  HELLOfrom mod_python import apache

  def accesshandler(req):
    apache.log_error("accesshandler")
    req.add_handler("PythonHandler","example::handler_3")
    return apache.OK

  def handler_1(req):
    apache.log_error("handler_1")
    req.content_type = 'text/plain'
    req.write("HELLO")
    return apache.OK

  def handler_2(req):
    apache.log_error("handler_2")
    return apache.OK

That is, the content as returned by handler_1(), followed by the contents of the example.py file.

If instead the URL wasn't 'example.py' but say 'other.py' with that not existing, get back:

  HELLO
  OK

  The requested URL /~grahamd/add_handler/foo.py was not found on this server.

  Apache/2.0.51 (Unix) mod_python/3.2.5b Python/2.3 Server at localhost Port 8080

In some ways, this behaviour suggests that the behaviour whereby it raised an exception was probably a better way of handling the situtation anyway. Thus, maybe the documentation should instead be changed and the code left as is, or at least the 0 arguments changed to be NOTSLIENT to make it more obvious what it is doing.

The other option is to change the code to use SILENT, but then document the strange things that can result if the specified handler doesn't exist.

Comments??????

Comments

[Graham Dumpleton]

A related note against this issue with req.add_handler(), if you errornously do something like:

  def accesshandler(req):
    apache.log_error("accesshandler")
    req.add_handler("PythonHandler","")
    req.add_handler("PythonHandler","example::handler_2")
    return apache.OK

Ie., register the handler as an empty string, it doesn't complain, and further it stops any latter handlers being executed at all.

As far as I can tell at the moment this is because of a check in the HandlerDispatch member function of the Callback class in mod_python.apache. Ie.,

            while hlist.handler:

Normally hlist.handler would be Python None object for the end of the list and that is why it would stop in that case. Where you register an empty string it has the same effect of stopping iteration as it doesn't explicitly qualify the check as being for None object.

If one wanted to ensure that errors occurred whenever possible to highlight problems, it may be better to write the check as:

           while hlist.handler is not None:

That way if the string is set to empty by mistake, your would get an exception later on of:

  Mod_python error: "PythonHandler "

  Traceback (most recent call last):

    File "/System/Library/Frameworks/Python.framework/Versions/2.3/lib/python2.3/site-packages/mod_python/apache.py", line 287, in HandlerDispatch
      log=debug)

    File "/System/Library/Frameworks/Python.framework/Versions/2.3/lib/python2.3/site-packages/mod_python/apache.py", line 468, in import_module
      f, p, d = imp.find_module(parts[i], path)

  ImportError: No module named

You would have to realise the problem was the empty module name it tried to load, but at least it generates an exception rather than silently ignoring the remainder of the registered handlers.

[Graham Dumpleton]

In respect of SILENT and return of apache.DECLINED, if it is desired that it truly be silent like the documentation says it will be, then in CallBack.HandlerDispatch of mod_python.apache, then instead of:

                elif hlist.silent:
                    result = DECLINED

it probably should read:

                elif hlist.silent:
                    if result != OK:
                        result = DECLINED

By doing this, it means that a handler which could not be found, but which followed on from a previous handler, will not replace the OK result of the previous handler with DECLINED.

The only time the result would be set to DECLINED would be where the handler which could not be found was the first handler in a list of handlers. In that case, a subsequent handler could still set result to OK or generate some other result. The DECLINED result of the missing handler would only propogate back if there was no subsequent handler.

Thus the above would be the appropriate change were you to want it to be silent like the documentation says. I still perhaps feel that it shouldn't be silent though and it should generate an error, else the problem in the code wouldn't be picked up but would otherwise appear to run. Thus still vote for NOTSILENT. :-)

[Graham Dumpleton]

Am probably being bad sticking multiple problems against one bug report, but they are all related. :-)

That said, there is a third problem lurking in this code. With a .htaccess file of:

  SetHandler mod_python
  PythonAccessHandler example
  # PythonHandler example::handler_1
  PythonDebug On

Ie., use SetHandler, but don't define PythonHandler. If you now have an accesshandler() with:

  def accesshandler(req):
    apache.log_error("accesshandler")
    req.add_handler("PythonHandler","example::handler_1")
    return apache.OK

then Apache will core dump. The core dump isn't at the point that req.add_handler() is called but some later point when the content handler phase is processed (I think).

More later when I track it down.

[Graham Dumpleton]

Okay, got a fix for this third issue.

The problem lies in python_handler() function in mod_python.c, namely it has:

    /* create a hahdler list object */
    request_obj->hlo = (hlistobject *)MpHList_FromHLEntry(hle);

    /* add dynamically registered handlers, if any */
    if (dynhle) {
        MpHList_Append(request_obj->hlo, dynhle);
    }

In the example I created, 'hle' was 0 and 'dynhle' was non 0. When MpHList_FromHLEntry() was called it was being given 0. In that function, it would then call hlist_copy on the null pointer causing it subsequently crash.

First attempt at a fix was to not call hlist_copy() if it was a null pointer, but if that was done, it would then crash in MpHList_Append() because you were trying to append to list when there wasn't actually a list to append to.

Thus, correct fix is to change the above code to:

    if (!hle)
    {
        /* create a handler list object from dynamically registered handlers */
        request_obj->hlo = (hlistobject *)MpHList_FromHLEntry(dynhle);
    }
    else
    {
        /* create a handler list object */
        request_obj->hlo = (hlistobject *)MpHList_FromHLEntry(hle);

        /* add dynamically registered handlers, if any */
        if (dynhle) {
            MpHList_Append(request_obj->hlo, dynhle);
        }
    }

That is, if 'hle' is 0, create the handler list from 'dynhle' instead, else use existing code.

Note that case of both 'hle' and 'dynhle' was filtered out previously in the function.

I ran the test cases and it all passed with the above change. Test cases for the problems described in this bug report should perhaps be created.

[Graham Dumpleton]

To summarise the changes in the above into one spot so it is easier to see what is required:

1. If say that exception is the way to go, change documentation of req.add_handler(). Thus change:

    Note: There is no checking being done on the validity of the handler name. If you pass this function an invalid handler it will simply be ignored.

to something like:

    Note: If you pass this function an invalid handler, an exception will be generated at the time an attempt is made to find the handler.

That or just delete the note altogether.

2. Change code so that exception raised if handler string is empty. Thus in HandleDispatch of apache.py change:

    while hlist.handler:

to:

    while hlist.handler is not None:

3. Fix segfault when adding handler to empty list. Thus in python_handler() function in mod_python.c, change:

    /* create a hahdler list object */
    request_obj->hlo = (hlistobject *)MpHList_FromHLEntry(hle);

    /* add dynamically registered handlers, if any */
    if (dynhle) {
        MpHList_Append(request_obj->hlo, dynhle);
    }

to:

    if (!hle)
    {
        /* create a handler list object from dynamically registered handlers */
        request_obj->hlo = (hlistobject *)MpHList_FromHLEntry(dynhle);
    }
    else
    {
        /* create a handler list object */
        request_obj->hlo = (hlistobject *)MpHList_FromHLEntry(hle);

        /* add dynamically registered handlers, if any */
        if (dynhle) {
            MpHList_Append(request_obj->hlo, dynhle);
        }
    }

4. Just in case there was some way it could still be triggered if the above is done. Only allow faulty handler if marked as silent to propagate DECLINED if it is the first and only handler. Thus change in HandlerDispatch of apache.py:

                elif hlist.silent:
                    result = DECLINED

to:

                elif hlist.silent:
                    if result != OK:
                        result = DECLINED

5. To make intent of code clearer in as much as highlighting that exception will be raised if handler not found, change select lines of code in req_add_handler() of requestobject.c to use NOTSILENT instead of 0. Ie., from:

        hlist_append(self->request_rec->pool, self->hlo->head,
                     handler, dir, 0);

            hle = hlist_new(self->request_rec->pool, handler, dir, 0);

            hlist_append(self->request_rec->pool, hle, handler, dir, 0);

to:

        hlist_append(self->request_rec->pool, self->hlo->head,
                     handler, dir, NOTSILENT);

            hle = hlist_new(self->request_rec->pool, handler, dir, NOTSILENT);

            hlist_append(self->request_rec->pool, hle, handler, dir, NOTSILENT);

I know that I should be providing a patch, but right now my mod_python code base has various other changes in it related to the simple GIL issues and I would have to take them back out to get a clean patch. :-(

[Jim Gallacher]

Applied Graham's suggestions so all these related issues can be considered fixed.

Still need to write some unit tests before this issue is marked as resolved.

[Jim Gallacher]

Added unit tests.