[Mailman-Developers] [Patch] SMTPDirect.py wrongly closing the connection

[Barry A. Warsaw]

>>>>> "MM" == Marc MERLIN <marc_news@vasoftware.com> writes:

    MM> So, I have my mailman alpha 3 cvs with python 2.1 seemingly
    MM> working, and when I move a list with more than one subscriber
    MM> to it

Exactly how many subscribers are on your list?  Is it more than
SMTP_MAX_RCPTS?  I hope your answer is "yes"!

    MM> Apparently smtplib.py makes a delivery attempt without doing
    MM> EHLO or HELO first, and then users the size extention, which
    MM> is illegal before EHLO has been used.

Here's my theory, which only holds true if the #of subscribers >
SMTP_MAX_RCPTS.  If it's fewer, then I'm perplexed.

Let's forget about threaded delivery for now, which doesn't enter into
your picture anyway.

SMTPDirect tries to reuse a connection to the smtpd when delivering
multiple chunks.  The code in deliver() as it currently stands will do
the following:

- On the first chunk, it finds its socket isn't connected, so it
  connects, doing the EHLO/HELO dance
- Then it calls conn.sendmail() which does the MAIL FROM/RCPT TO/DATA
  dance
- Then the finally clause executes and a QUIT is issued.  The server
  closes the connection, as it must (RFC 2821).
- Now the next chunk comes along and we find the socket is not
  connected, so we re-connect.  Note that we're using the same
  smtplib.SMTP object, but we're getting a new socket connection
- The state in the SMTP object thinks it's already issued an EHLO and
  so doesn't issue it again.  Instead, when this time it calls
  sendmail() it jumps right to MAIL FROM and boom, you're hosed.

Your patch eliminates the QUIT in the finally clause and so when the
the first transaction in the session completes, the session won't be
QUIT, the socket won't be closed, and the second sendmail() call
succeeds.

Interestingly enough, it looks like Postfix isn't bothered by the lack
of EHLO/HELO at the start of the second session.  This behavior
appears to be controlled by the smtpd_helo_required config variable
which seems to default to "no".  Can you guess why I never noticed
this bug before? :)

Your patch clearly gets to the heart of the matter: in a multiple
transaction session, we mustn't issue a QUIT until the session is
over.  So SMTPDirect.py is broken.  I'm not sure your patch is
entirely correct -- I need to trace the code through both the
non-threaded and threaded delivery paths and think about the error
conditions.  Probably the whole try/finally bit can be removed, and
.quit() needs to be called when we're really done with the
connection.  Attached is a patch that I think will do the trick, but I
haven't tested it yet.

Now, if you tell me that the # of recips is < SMTP_MAX_RCPTS, then
none of this analysis is relevant, and I can't see how you're getting
this problem. ;)

    MM> I put back a pass instead, and I'm leaving for a snowboard
    MM> trip today, so I think I'll leave the lists alone until I come
    MM> back sunday :-)

Oh sure, rub it in!  We're still unseasonably warm here in DC, even if
we have gotten below 70 deg F the last few days.  No sign of snow so
far, so I hope you think about Mailman on all the lifts up the
mountain. :) 'Course, if we get some decent snow in February when I'm
supposed to go, then I take it all back!

warm-ly y'rs,
-Barry

P.S. Python 2.2 rc 1 release tomorrow, er, later today.  Hopefully
some Mailman catch up time this weekend.

-------------------- snip snip --------------------
Index: SMTPDirect.py
===================================================================
RCS file: /cvsroot/mailman/mailman/Mailman/Handlers/SMTPDirect.py,v
retrieving revision 2.10
diff -u -r2.10 SMTPDirect.py
--- SMTPDirect.py	2001/11/21 22:57:27	2.10
+++ SMTPDirect.py	2001/12/14 07:58:31
@@ -98,7 +98,7 @@
         conn = smtplib.SMTP()
         for chunk in chunks:
             deliver(envsender, msgtext, chunk, refused, conn)
-        conn.close()
+        conn.quit()
     # Log the successful post
     t1 = time.time()
     d = MsgSafeDict(msg, {'time'    : t1-t0,
@@ -242,7 +242,7 @@
     for t, (threadfailures, conn) in threads.items():
         t.join()
         failures.update(threadfailures)
-        conn.close()
+        conn.quit()
     # All threads have exited
     threads.clear()
 
@@ -269,12 +269,8 @@
         # connected or not. :(
         if not getattr(conn, 'sock', 0):
             conn.connect(mm_cfg.SMTPHOST, mm_cfg.SMTPPORT)
-        try:
-            # make sure the connect happens, which won't be done by the
-            # constructor if SMTPHOST is false
-            refused = conn.sendmail(envsender, recips, msgtext)
-        finally:
-            conn.quit()
+        # Send the message
+        refused = conn.sendmail(envsender, recips, msgtext)
     except smtplib.SMTPRecipientsRefused, e:
         refused = e.recipients
     # MTA not responding, or other socket problems, or any other kind of