bpo-36348: IMAP4.logout() doesn't ignore exc (GH-12411)

The imap.IMAP4.logout() method no longer ignores silently arbitrary
exceptions.

Changes:

* The IMAP4.logout() method now expects a "BYE" untagged response,
  rather than relying on _check_bye() which raises a self.abort()
  exception.
* IMAP4.__exit__() now does nothing if the client already logged out.
* Add more debug info if test_logout() tests fail.

5 files changed, 28 insertions, 13 deletions

diff --git a/Doc/library/imaplib.rst b/Doc/library/imaplib.rst
index d0709f8b67..f027f82dde 100644
--- a/Doc/library/imaplib.rst
+++ b/Doc/library/imaplib.rst
@@ -327,6 +327,9 @@ An :class:`IMAP4` instance has the following methods:
 
    Shutdown connection to server. Returns server ``BYE`` response.
 
+   .. versionchanged:: 3.8
+      The method no longer ignores silently arbitrary exceptions.
+
 
 .. method:: IMAP4.lsub(directory='""', pattern='*')
 
diff --git a/Doc/whatsnew/3.8.rst b/Doc/whatsnew/3.8.rst
index 39a0da5e61..f866f9ccb8 100644
--- a/Doc/whatsnew/3.8.rst
+++ b/Doc/whatsnew/3.8.rst
@@ -709,6 +709,9 @@ Changes in Python behavior
 Changes in the Python API
 -------------------------
 
+* The :meth:`imap.IMAP4.logout` method no longer ignores silently arbitrary
+  exceptions.
+
 * The function :func:`platform.popen` has been removed, it was deprecated since
   Python 3.3: use :func:`os.popen` instead.
 
diff --git a/Lib/imaplib.py b/Lib/imaplib.py
index dd237f7704..341ee25ae9 100644
--- a/Lib/imaplib.py
+++ b/Lib/imaplib.py
@@ -272,6 +272,9 @@ class IMAP4:
         return self
 
     def __exit__(self, *args):
+        if self.state == "LOGOUT":
+            return
+
         try:
             self.logout()
         except OSError:
@@ -625,11 +628,8 @@ class IMAP4:
         Returns server 'BYE' response.
         """
         self.state = 'LOGOUT'
-        try: typ, dat = self._simple_command('LOGOUT')
-        except: typ, dat = 'NO', ['%s: %s' % sys.exc_info()[:2]]
+        typ, dat = self._simple_command('LOGOUT')
         self.shutdown()
-        if 'BYE' in self.untagged_responses:
-            return 'BYE', self.untagged_responses['BYE']
         return typ, dat
 
 
@@ -1012,16 +1012,17 @@ class IMAP4:
 
 
     def _command_complete(self, name, tag):
+        logout = (name == 'LOGOUT')
         # BYE is expected after LOGOUT
-        if name != 'LOGOUT':
+        if not logout:
             self._check_bye()
         try:
-            typ, data = self._get_tagged_response(tag)
+            typ, data = self._get_tagged_response(tag, expect_bye=logout)
         except self.abort as val:
             raise self.abort('command: %s => %s' % (name, val))
         except self.error as val:
             raise self.error('command: %s => %s' % (name, val))
-        if name != 'LOGOUT':
+        if not logout:
             self._check_bye()
         if typ == 'BAD':
             raise self.error('%s command error: %s %s' % (name, typ, data))
@@ -1117,7 +1118,7 @@ class IMAP4:
         return resp
 
 
-    def _get_tagged_response(self, tag):
+    def _get_tagged_response(self, tag, expect_bye=False):
 
         while 1:
             result = self.tagged_commands[tag]
@@ -1125,9 +1126,15 @@ class IMAP4:
                 del self.tagged_commands[tag]
                 return result
 
+            if expect_bye:
+                typ = 'BYE'
+                bye = self.untagged_responses.pop(typ, None)
+                if bye is not None:
+                    # Server replies to the "LOGOUT" command with "BYE"
+                    return (typ, bye)
+
             # If we've seen a BYE at this point, the socket will be
             # closed, so report the BYE now.
-
             self._check_bye()
 
             # Some have reported "unexpected response" exceptions.
diff --git a/Lib/test/test_imaplib.py b/Lib/test/test_imaplib.py
index aec36af6c5..9305e47ee9 100644
--- a/Lib/test/test_imaplib.py
+++ b/Lib/test/test_imaplib.py
@@ -470,8 +470,8 @@ class NewIMAPTestsMixin():
         self.assertEqual(typ, 'OK')
         self.assertEqual(data[0], b'LOGIN completed')
         typ, data = client.logout()
-        self.assertEqual(typ, 'BYE')
-        self.assertEqual(data[0], b'IMAP4ref1 Server logging out')
+        self.assertEqual(typ, 'BYE', (typ, data))
+        self.assertEqual(data[0], b'IMAP4ref1 Server logging out', (typ, data))
         self.assertEqual(client.state, 'LOGOUT')
 
     def test_lsub(self):
@@ -937,7 +937,7 @@ class RemoteIMAPTest(unittest.TestCase):
         with transient_internet(self.host):
             rs = self.server.logout()
             self.server = None
-            self.assertEqual(rs[0], 'BYE')
+            self.assertEqual(rs[0], 'BYE', rs)
 
 
 @unittest.skipUnless(ssl, "SSL not available")
@@ -995,7 +995,7 @@ class RemoteIMAP_SSLTest(RemoteIMAPTest):
         with transient_internet(self.host):
             _server = self.imap_class(self.host, self.port)
             rs = _server.logout()
-            self.assertEqual(rs[0], 'BYE')
+            self.assertEqual(rs[0], 'BYE', rs)
 
     def test_ssl_context_certfile_exclusive(self):
         with transient_internet(self.host):
diff --git a/Misc/NEWS.d/next/Library/2019-03-18-16-16-55.bpo-36348.E0w_US.rst b/Misc/NEWS.d/next/Library/2019-03-18-16-16-55.bpo-36348.E0w_US.rst
new file mode 100644
index 0000000000..2320b4c05b
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2019-03-18-16-16-55.bpo-36348.E0w_US.rst
@@ -0,0 +1,2 @@
+The :meth:`imap.IMAP4.logout` method no longer ignores silently arbitrary
+exceptions.