[Tutor] sockets, files, threads

[Danny Yoo]

Hi Marilyn,


[Long program comments ahead.  Please forgive me if some of the comments
are overbearing; I'm trying to get over a cold, and I'm very grumpy.
*grin*]


Some comments on the code follow.  I'll be focusing on:

> http://www.maildance.com/python/doorman/py_daemon.py

One of the import statements:

###
from signal import *
###

may not be safe.  According to:

    http://docs.python.org/tut/node8.html#SECTION008410000000000000000

"""Note that in general the practice of importing * from a module or
package is frowned upon, since it often causes poorly readable code.
However, it is okay to use it to save typing in interactive sessions, and
certain modules are designed to export only names that follow certain
patterns."""




There's a block of code in Server.run() that looks problematic:

###
        while 1:
            if log.level & log.calls:
                log.it("fd%d:py_daemon.py: Waiting ...", self.descriptor)
            try:
                client_socket, client_addr = self.server_socket.accept()
            except socket.error, msg:
                time.sleep(.5)
                continue
            except (EOFError, KeyboardInterrupt):
                self.close_up()
            Spawn(client_socket).start()
###

The potentially buggy line is the last one:

    Spawn(client_socket).start()


The problem is that, as part of program flow, it appears to run after the
try block.  But in one particular case of program flow, 'client_socket'
will not be set to a valid value.  It is better to put that statement a
few lines up, right where 'client_socket' is initialized.  Like this:

###
            try:
                client_socket, client_addr = self.server_socket.accept()
                Spawn(client_socket).start()
            except socket.error, msg:
                time.sleep(.5)
            except (EOFError, KeyboardInterrupt):
                self.close_up()
###

Not only does this make it more clear where 'client_socket' is being used,
but it ends up making the code shorter.  I've dropped the 'continue'
statement, as it becomes superfluous when the Spawn() moves into the try's
body.

In fact, the placement of that statement may account partially for the
error message that you were running into earlier.  The earlier error
message:

###
close failed: [Errno 9] Bad file descriptor
###

can easliy occur if there's an EOFError or KeyboardInterrupt under the
original code.  And although KeyboardInterrupt might be unusual, I'm not
so sure if EOFError is.



Furthermore, the 'except' blocks can emit more debugging information.
You mentioned earlier that you're not getting good error tracebacks when
exceptions occur.  Let's fix that.

Use the 'traceback' module here for all except blocks in your program.
This will ensure that you get a good stack trace that we can inspect.

Here's what the code block looks like, with those adjustments:

###
            try:
                client_socket, client_addr = self.server_socket.accept()
                Spawn(client_socket).start()
            except socket.error, msg:
                time.sleep(.5)
                log.it(traceback.format_exc())
            except (EOFError, KeyboardInterrupt):
                self.close_up()
                log.it(traceback.format_exc())
###

I'm using Python 2.4's format_exc() function to get the stack trace as a
string.  If this version is not available to you, you can use this
workaround format_exc():

###
import StringIO
import traceback

def format_exc():
    """Returns the exception as a string."""
    f = StringIO.StringIO()
    traceback.print_exc(file = f)
    return f.getvalue()
###

Once you've made the correction and augmented all the except blocks with
traceback logging, try running your program again.  We should then be
better able to trace down the root cause of those errors.


Let me just look through a little bit more, just to see what else we can
pick out.

>From what I read of the code so far, I believe that a lot of the locking
that the code is doing is also superfluous.  You do not need to lock local
variables, nor do you have to usually lock things during object
initialization.  For example, FileReader.__init__()  has the following
code:

###
class FileReader(TokenReader):
    def __init__(self, file_socket):
        self.local_name = file_socket.local_name
        self.freader_name = '/tmp/fsf%s' % self.local_name
        file_lock.acquire()
        self.freader = open(self.freader_name, "w+")
        file_lock.release()
###

The locks around the open() calls are unnecessary: you do not need to
synchronize file opening here, as there's no way for another thread to get
into the same initializer of the same instance.

In fact, as far as I can tell, none of the Spawn() threads are
communicating with each other.  As long as your threads are working
independently of each other --- and as long as they are not writing to
global variables --- you do not need locks.

In summary, a lot of that locking code isn't doing anything, and may
actually be a source of problems if we're not careful.



The other classes are complex enough that I actually don't trust either of
them.  I am a very paranoid person.  *grin*

FileReader appears to reimplement a temporary-file implementation: I'd
strongly recommend using tempfile.TemporaryFile instead of reimplementing
its functionality.  Either that, or use StringIO() to suck the whole file
into memory.  How large do you expect a single email message to be?

There appears to be missing symmetry between the open() and the close()
calls in the code, and that's a sign of possible trouble.  I try to
structure most file-handling code with the following structure:

###
f = open(somefile)
try:
    ... do some stuff with f
finally:
    f.close()
###

Code that doesn't follow this idiom might be susceptable to resource
leakage: we might forget to close() something.  This idiom's probably not
as important in Python, since we have garbage collection, but I still try
to follow it to prevent things like running out of file handles too
quickly.  *grin*


The eval() architecture that Spawn uses to dispatch to either
calls.acl_rcpt, calls.route_mail, calls.route_errors, calls.doorman, or
calls.doorman_errors is dangerous.  I know that in prior email, you
mentioned that you were using eval().

I had misgivings then, and I definitely have them now: the program calls
eval() based on stuff that's coming from a socket, and that's just asking
for trouble.

I strongly recommend modifying Spawn.self() to something like this:

######
def tokenToDispatchFunction(self, prog):
    """Given a command, returns the correct dispatch function.  If prog
    is incorrect, throws a LookupError."""
    dispatchTable = { 'acl_rept' : calls.acl_rept,
                      'route_errors' : calls.route_errors,
                      'doorman' : calls.doorman,
                      'doorman_errors' : calls.doorman_errors'
                    }
    return dispatchTable[token].main


def run(self):
     """Executes one of the calls program."""
     argv = []
     dispatch = self.tokenToDispatchFunction(self.exim_io.call)
     dispatch(argv)
######

I've oversimplified ("broken") the code here; you'll need to reinsert the
argument-construction stuff and the logging.  But other than that, this
should have the same external-command calling functionality as the
original code, with additional safety.

The danger is that the tokens that we are reading from the socket can be
arbitrarily formed.  One potential token that can be written might be:

###
evil = '__name__[__import__("os").system("ls"+chr(32)+"/")].py'
###

'evil' is nonsensical, but according to the tokenization code, this is a
single token, since it does not contain spaces.


Imagine that that weird string is the first token that
FileSocket.find_call() returns to us.  Back in Spawn.run(), under the
original code, we end up trying to evaluate a string that looks like:

    'call.__name__[__import__("os").system("ls"+chr(32)+"/")].main(argv)'

which in fact will break with a runtime error, but not before doing
mischief.  This is exactly the kind of devious exploit that eval() opens
up.  It's hard to close the bottle after the genie's out.


The revised code that uses a dispatchTable method is much more resiliant
to this kind of attack.  If someone tries to send malformed commands, the
worse that can happen is a KeyError.  More than that, if we look back at
that tokenToDispatchFunction() again:

###
def tokenToDispatchFunction(self, prog):
    dispatchTable = { 'acl_rept' : calls.acl_rept,
                      'route_errors' : calls.route_errors,
                      'doorman' : calls.doorman,
                      'doorman_errors' : calls.doorman_errors'
               }
    return dispatchTable[token].main
###

it's much more clear what possible calls can happen.


Anyway, my eyes are going dim, so I'd better stop right now.  *grin* Sorry
for taking so much space; I hope this helps!