Bug #1557232: fix seg fault with def f((((x)))) and def f(((x),)).

These tests should be improved.  Hopefully this fixes variations when
flipping back and forth between fpdef and fplist.

Backport candidate.

3 files changed, 117 insertions, 2 deletions

diff --git a/Lib/test/test_complex_args.py b/Lib/test/test_complex_args.py
new file mode 100644
index 0000000..c6d50a9
--- /dev/null
+++ b/Lib/test/test_complex_args.py
@@ -0,0 +1,91 @@
+
+import unittest
+from test import test_support
+
+class ComplexArgsTestCase(unittest.TestCase):
+
+    def check(self, func, expected, *args):
+        self.assertEqual(func(*args), expected)
+
+    # These functions are tested below as lambdas too.  If you add a function test,
+    # also add a similar lambda test.
+
+    def test_func_parens_no_unpacking(self):
+        def f(((((x))))): return x
+        self.check(f, 1, 1)
+        # Inner parens are elided, same as: f(x,)
+        def f(((x)),): return x
+        self.check(f, 2, 2)
+
+    def test_func_1(self):
+        def f(((((x),)))): return x
+        self.check(f, 3, (3,))
+        def f(((((x)),))): return x
+        self.check(f, 4, (4,))
+        def f(((((x))),)): return x
+        self.check(f, 5, (5,))
+        def f(((x),)): return x
+        self.check(f, 6, (6,))
+
+    def test_func_2(self):
+        def f(((((x)),),)): return x
+        self.check(f, 2, ((2,),))
+
+    def test_func_3(self):
+        def f((((((x)),),),)): return x
+        self.check(f, 3, (((3,),),))
+
+    def test_func_complex(self):
+        def f((((((x)),),),), a, b, c): return x, a, b, c
+        self.check(f, (3, 9, 8, 7), (((3,),),), 9, 8, 7)
+
+        def f(((((((x)),)),),), a, b, c): return x, a, b, c
+        self.check(f, (3, 9, 8, 7), (((3,),),), 9, 8, 7)
+
+        def f(a, b, c, ((((((x)),)),),)): return a, b, c, x
+        self.check(f, (9, 8, 7, 3), 9, 8, 7, (((3,),),))
+
+    # Duplicate the tests above, but for lambda.  If you add a lambda test,
+    # also add a similar function test above.
+
+    def test_lambda_parens_no_unpacking(self):
+        f = lambda (((((x))))): x
+        self.check(f, 1, 1)
+        # Inner parens are elided, same as: f(x,)
+        f = lambda ((x)),: x
+        self.check(f, 2, 2)
+
+    def test_lambda_1(self):
+        f = lambda (((((x),)))): x
+        self.check(f, 3, (3,))
+        f = lambda (((((x)),))): x
+        self.check(f, 4, (4,))
+        f = lambda (((((x))),)): x
+        self.check(f, 5, (5,))
+        f = lambda (((x),)): x
+        self.check(f, 6, (6,))
+
+    def test_lambda_2(self):
+        f = lambda (((((x)),),)): x
+        self.check(f, 2, ((2,),))
+
+    def test_lambda_3(self):
+        f = lambda ((((((x)),),),)): x
+        self.check(f, 3, (((3,),),))
+
+    def test_lambda_complex(self):
+        f = lambda (((((x)),),),), a, b, c: (x, a, b, c)
+        self.check(f, (3, 9, 8, 7), (((3,),),), 9, 8, 7)
+
+        f = lambda ((((((x)),)),),), a, b, c: (x, a, b, c)
+        self.check(f, (3, 9, 8, 7), (((3,),),), 9, 8, 7)
+
+        f = lambda a, b, c, ((((((x)),)),),): (a, b, c, x)
+        self.check(f, (9, 8, 7, 3), 9, 8, 7, (((3,),),))
+
+
+def test_main():
+    test_support.run_unittest(ComplexArgsTestCase)
+
+if __name__ == "__main__":
+    test_main()
diff --git a/Misc/NEWS b/Misc/NEWS
index e93a80e..d7f1e75 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -12,6 +12,8 @@ What's New in Python 2.6 alpha 1?
 Core and builtins
 -----------------
 
+- Bug #1557232: fix seg fault with def f((((x)))) and def f(((x),)).
+
 - Fix %zd string formatting on Mac OS X so it prints negative numbers.
 
 - Allow exception instances to be directly sliced again.
diff --git a/Python/ast.c b/Python/ast.c
index 9e0c184..f490268 100644
--- a/Python/ast.c
+++ b/Python/ast.c
@@ -560,10 +560,17 @@ compiler_complex_args(struct compiling *c, const node *n)
     if (!args)
         return NULL;
 
+    /* fpdef: NAME | '(' fplist ')'
+       fplist: fpdef (',' fpdef)* [',']
+    */
     REQ(n, fplist);
     for (i = 0; i < len; i++) {
-        const node *child = CHILD(CHILD(n, 2*i), 0);
+        const node *fpdef_node = CHILD(n, 2*i);
+        const node *child;
         expr_ty arg;
+set_name:
+        /* fpdef_node is either a NAME or an fplist */
+        child = CHILD(fpdef_node, 0);
         if (TYPE(child) == NAME) {
                 if (!strcmp(STR(child), "None")) {
                         ast_error(child, "assignment to None");
@@ -573,7 +580,17 @@ compiler_complex_args(struct compiling *c, const node *n)
                        child->n_col_offset, c->c_arena);
             }
         else {
-            arg = compiler_complex_args(c, CHILD(CHILD(n, 2*i), 1));
+            assert(TYPE(fpdef_node) == fpdef);
+            /* fpdef_node[0] is not a name, so it must be a '(', get CHILD[1] */
+            child = CHILD(fpdef_node, 1);
+            assert(TYPE(child) == fplist);
+            /* NCH == 1 means we have (x), we need to elide the extra parens */
+            if (NCH(child) == 1) {
+                fpdef_node = CHILD(child, 0);
+                assert(TYPE(fpdef_node) == fpdef);
+                goto set_name;
+            }
+            arg = compiler_complex_args(c, child);
         }
         asdl_seq_SET(args, i, arg);
     }
@@ -631,6 +648,7 @@ ast_for_arguments(struct compiling *c, const node *n)
         ch = CHILD(n, i);
         switch (TYPE(ch)) {
             case fpdef:
+            handle_fpdef:
                 /* XXX Need to worry about checking if TYPE(CHILD(n, i+1)) is
                    anything other than EQUAL or a comma? */
                 /* XXX Should NCH(n) check be made a separate check? */
@@ -656,7 +674,11 @@ ast_for_arguments(struct compiling *c, const node *n)
                         asdl_seq_SET(args, k++, compiler_complex_args(c, ch));
                     } else {
                         /* def foo((x)): setup for checking NAME below. */
+                        /* Loop because there can be many parens and tuple
+                           unpacking mixed in. */
                         ch = CHILD(ch, 0);
+                        assert(TYPE(ch) == fpdef);
+                        goto handle_fpdef;
                     }
                 }
                 if (TYPE(CHILD(ch, 0)) == NAME) {