Fixed #25677 -- Prevented decoding errors in/after Popen calls

Thanks Gavin Wahl for the report and Tim Graham for the review.

4 files changed, 54 insertions, 43 deletions

diff --git a/django/core/management/commands/makemessages.py b/django/core/management/commands/makemessages.py
index 866e325816..1fe17cad7f 100644
--- a/django/core/management/commands/makemessages.py
+++ b/django/core/management/commands/makemessages.py
@@ -16,7 +16,6 @@ from django.core.management.base import BaseCommand, CommandError
 from django.core.management.utils import (
     find_command, handle_extensions, popen_wrapper,
 )
-from django.utils import six
 from django.utils._os import upath
 from django.utils.encoding import DEFAULT_LOCALE_ENCODING, force_str
 from django.utils.functional import cached_property
@@ -35,26 +34,6 @@ def check_programs(*programs):
                     "gettext tools 0.15 or newer installed." % program)
 
 
-def gettext_popen_wrapper(args, os_err_exc_type=CommandError, stdout_encoding="utf-8"):
-    """
-    Makes sure text obtained from stdout of gettext utilities is Unicode.
-    """
-    # This both decodes utf-8 and cleans line endings. Simply using
-    # popen_wrapper(universal_newlines=True) doesn't properly handle the
-    # encoding. This goes back to popen's flaky support for encoding:
-    # https://bugs.python.org/issue6135. This is a solution for #23271, #21928.
-    # No need to do anything on Python 2 because it's already a byte-string there.
-    manual_io_wrapper = six.PY3 and stdout_encoding != DEFAULT_LOCALE_ENCODING
-
-    stdout, stderr, status_code = popen_wrapper(args, os_err_exc_type=os_err_exc_type,
-                                                universal_newlines=not manual_io_wrapper)
-    if manual_io_wrapper:
-        stdout = io.TextIOWrapper(io.BytesIO(stdout), encoding=stdout_encoding).read()
-    if six.PY2:
-        stdout = stdout.decode(stdout_encoding)
-    return stdout, stderr, status_code
-
-
 @total_ordering
 class TranslatableFile(object):
     def __init__(self, dirpath, file_name, locale_dir):
@@ -334,7 +313,7 @@ class Command(BaseCommand):
     def gettext_version(self):
         # Gettext tools will output system-encoded bytestrings instead of UTF-8,
         # when looking up the version. It's especially a problem on Windows.
-        out, err, status = gettext_popen_wrapper(
+        out, err, status = popen_wrapper(
             ['xgettext', '--version'],
             stdout_encoding=DEFAULT_LOCALE_ENCODING,
         )
@@ -357,7 +336,7 @@ class Command(BaseCommand):
             if not os.path.exists(potfile):
                 continue
             args = ['msguniq'] + self.msguniq_options + [potfile]
-            msgs, errors, status = gettext_popen_wrapper(args)
+            msgs, errors, status = popen_wrapper(args)
             if errors:
                 if status != STATUS_OK:
                     raise CommandError(
@@ -510,7 +489,7 @@ class Command(BaseCommand):
             input_files_list.flush()
             args.extend(['--files-from', input_files_list.name])
             args.extend(self.xgettext_options)
-            msgs, errors, status = gettext_popen_wrapper(args)
+            msgs, errors, status = popen_wrapper(args)
 
         if errors:
             if status != STATUS_OK:
@@ -553,7 +532,7 @@ class Command(BaseCommand):
 
         if os.path.exists(pofile):
             args = ['msgmerge'] + self.msgmerge_options + [pofile, potfile]
-            msgs, errors, status = gettext_popen_wrapper(args)
+            msgs, errors, status = popen_wrapper(args)
             if errors:
                 if status != STATUS_OK:
                     raise CommandError(
@@ -572,7 +551,7 @@ class Command(BaseCommand):
 
         if self.no_obsolete:
             args = ['msgattrib'] + self.msgattrib_options + ['-o', pofile, pofile]
-            msgs, errors, status = gettext_popen_wrapper(args)
+            msgs, errors, status = popen_wrapper(args)
             if errors:
                 if status != STATUS_OK:
                     raise CommandError(
diff --git a/django/core/management/utils.py b/django/core/management/utils.py
index 53290e36d7..08efe11c87 100644
--- a/django/core/management/utils.py
+++ b/django/core/management/utils.py
@@ -10,24 +10,22 @@ from django.utils.encoding import DEFAULT_LOCALE_ENCODING, force_text
 from .base import CommandError
 
 
-def popen_wrapper(args, os_err_exc_type=CommandError, universal_newlines=True):
+def popen_wrapper(args, os_err_exc_type=CommandError, stdout_encoding='utf-8'):
     """
     Friendly wrapper around Popen.
 
     Returns stdout output, stderr output and OS status code.
     """
     try:
-        p = Popen(args, shell=False, stdout=PIPE, stderr=PIPE,
-                close_fds=os.name != 'nt', universal_newlines=universal_newlines)
+        p = Popen(args, shell=False, stdout=PIPE, stderr=PIPE, close_fds=os.name != 'nt')
     except OSError as e:
-        strerror = force_text(e.strerror, DEFAULT_LOCALE_ENCODING,
-                              strings_only=True)
+        strerror = force_text(e.strerror, DEFAULT_LOCALE_ENCODING, strings_only=True)
         six.reraise(os_err_exc_type, os_err_exc_type('Error executing %s: %s' %
                     (args[0], strerror)), sys.exc_info()[2])
     output, errors = p.communicate()
     return (
-        output,
-        force_text(errors, DEFAULT_LOCALE_ENCODING, strings_only=True),
+        force_text(output, stdout_encoding, strings_only=True, errors='strict'),
+        force_text(errors, DEFAULT_LOCALE_ENCODING, strings_only=True, errors='replace'),
         p.returncode
     )
 
diff --git a/tests/i18n/commands/locale/ko/LC_MESSAGES/django.po b/tests/i18n/commands/locale/ko/LC_MESSAGES/django.po
new file mode 100644
index 0000000000..30ed4f1bd9
--- /dev/null
+++ b/tests/i18n/commands/locale/ko/LC_MESSAGES/django.po
@@ -0,0 +1,24 @@
+# This file intentionally contains a wrong msgstr that will produce
+# a msgfmt error.
+# Copyright (C) YEAR THE PACKAGE'S COPYRIGHT HOLDER
+# This file is distributed under the same license as the PACKAGE package.
+# FIRST AUTHOR <EMAIL@ADDRESS>, YEAR.
+#
+msgid ""
+msgstr ""
+"Project-Id-Version: PACKAGE VERSION\n"
+"Report-Msgid-Bugs-To: \n"
+"POT-Creation-Date: 2015-11-04 12:01-0700\n"
+"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
+"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
+"Language-Team: LANGUAGE <LL@li.org>\n"
+"MIME-Version: 1.0\n"
+"Content-Type: text/plain; charset=UTF-8\n"
+"Content-Transfer-Encoding: 8bit\n"
+"Language: \n"
+"Plural-Forms: nplurals=2; plural=(n != 1);\n"
+
+#: foo/password_validation.py:17
+#, python-brace-format
+msgid "Your password must contain a symbol: {symbols}"
+msgstr "[Ẏǿŭř ƥȧşşẇǿřḓ ḿŭşŧ ƈǿƞŧȧīƞ ȧ şẏḿƀǿŀ: {şẏḿƀǿŀş} ΐΰϖΐ ϖΐẛϕϐ]"
diff --git a/tests/i18n/test_compilation.py b/tests/i18n/test_compilation.py
index e784b0a991..8508ecb855 100644
--- a/tests/i18n/test_compilation.py
+++ b/tests/i18n/test_compilation.py
@@ -1,4 +1,5 @@
 # -*- coding: utf-8 -*-
+from __future__ import unicode_literals
 
 import gettext as gettext_module
 import os
@@ -12,7 +13,7 @@ from django.core.management import (
 from django.core.management.utils import find_command
 from django.test import SimpleTestCase, override_settings
 from django.test.utils import captured_stderr, captured_stdout
-from django.utils import translation
+from django.utils import six, translation
 from django.utils._os import upath
 from django.utils.encoding import force_text
 from django.utils.six import StringIO
@@ -154,17 +155,26 @@ class ExcludedLocaleCompilationTests(MessageCompilationTests):
 
 
 class CompilationErrorHandling(MessageCompilationTests):
-
-    LOCALE = 'ja'
-    MO_FILE = 'locale/%s/LC_MESSAGES/django.mo' % LOCALE
-
-    def setUp(self):
-        super(CompilationErrorHandling, self).setUp()
-        self.addCleanup(self.rmfile, os.path.join(self.test_dir, self.MO_FILE))
-
     def test_error_reported_by_msgfmt(self):
+        # po file contains wrong po formatting.
+        mo_file = 'locale/ja/LC_MESSAGES/django.mo'
+        self.addCleanup(self.rmfile, os.path.join(self.test_dir, mo_file))
         with self.assertRaises(CommandError):
-            call_command('compilemessages', locale=[self.LOCALE], stdout=StringIO())
+            call_command('compilemessages', locale=['ja'], verbosity=0)
+
+    def test_msgfmt_error_including_non_ascii(self):
+        # po file contains invalid msgstr content (triggers non-ascii error content).
+        mo_file = 'locale/ko/LC_MESSAGES/django.mo'
+        self.addCleanup(self.rmfile, os.path.join(self.test_dir, mo_file))
+        if six.PY2:
+            # Various assertRaises on PY2 don't support unicode error messages.
+            try:
+                call_command('compilemessages', locale=['ko'], verbosity=0)
+            except CommandError as err:
+                self.assertIn("'�' cannot start a field name", six.text_type(err))
+        else:
+            with self.assertRaisesMessage(CommandError, "'�' cannot start a field name"):
+                call_command('compilemessages', locale=['ko'], verbosity=0)
 
 
 class ProjectAndAppTests(MessageCompilationTests):