bpo-40355: ast.literal_eval rejects malformed Dict nodes by curtisbucher · Pull Request #19868 · python/cpython

curtisbucher · 2020-05-02T21:48:11Z

This doesn't fix ast.unparse.

https://bugs.python.org/issue40355

remilapeyre

Hi @curtisbucher, the patch looks appropriate but if the length must be known it seems to me like you need to make sure that keys and values are actual sequences and not just iterables, this used to work but now doesn't:

from ast import Constant, Dict, literal_eval, unparse
keys = iter([Constant('a')])
values = iter([Constant('b')])
nasty_dict = Dict(keys=keys, values=values)
literal_eval(nasty_dict)

Here's the difference between 3.8 and this patch:

➜  cpython git:(bpo-40355) python3
Python 3.8.2 (v3.8.2:7b3ab5921f, Feb 24 2020, 17:52:18) 
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from ast import Constant, Dict, literal_eval, unparse
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'unparse' from 'ast' (/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/ast.py)
>>> keys = iter([Constant('a')])
>>> values = iter([Constant('b')])
>>> nasty_dict = Dict(keys=keys, values=values)
>>> literal_eval(nasty_dict)
{'a': 'b'}
>>> ^D
➜  cpython git:(bpo-40355) ./python
Python 3.9.0a6+ (heads/bpo-40355:36eb5b8c4b, May  2 2020, 23:57:18) 
[Clang 11.0.3 (clang-1103.0.32.29)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from ast import Constant, Dict, literal_eval, unparse
>>> keys = iter([Constant('a')])
>>> values = iter([Constant('b')])
>>> nasty_dict = Dict(keys=keys, values=values)
>>> literal_eval(nasty_dict)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/remi/src/cpython/Lib/ast.py", line 104, in literal_eval
    return _convert(node_or_string)
  File "/Users/remi/src/cpython/Lib/ast.py", line 91, in _convert
    if len(node.keys) != len(node.values):
TypeError: object of type 'list_iterator' has no len()
>>> ^D

brandtbucher · 2020-05-02T22:16:31Z

...this used to work but now doesn't:

Hm, this seems like a somewhat unusual pattern. Any examples out in the wild of clients generating ASTs with consumable iterators of children?

Either way, I agree that it's easy enough to be defensive here, so we might as well.

brandtbucher · 2020-05-02T22:19:07Z

+            if len(node.keys) != len(node.values):
+                raise ValueError(f'malformed node or string: {node!r}')
            return dict(zip(map(_convert, node.keys),
                            map(_convert, node.values)))


Collecting node.keys and node.values into lists should be fine:

Suggested change

if len(node.keys) != len(node.values):

raise ValueError(f'malformed node or string: {node!r}')

return dict(zip(map(_convert, node.keys),

map(_convert, node.values)))

keys, values = list(node.keys), list(node.values)

if len(keys) != len(values):

raise ValueError(f'malformed node or string: {node!r}')

return dict(zip(map(_convert, keys),

map(_convert, values)))

isidentical · 2020-05-03T10:31:58Z

the patch looks appropriate but if the length must be known it seems to me like you need to make sure that keys and values are actual sequences and not just iterables

I disagree. That was a side effect which used to work, and now it isn't. keys and values are defined as an asdl sequence, and that corresponds to list type in python. IMHO we should just assume those are lists and do our work.

remilapeyre · 2020-05-03T10:54:02Z

IMHO we should just assume those are lists and do our work.

I would have expected it to raise ValueError on initilization then. Should this be changed or we just suppose that nobody would do that?

isidentical · 2020-05-03T11:51:05Z

I would have expected it to raise ValueError on initilization then.

Well it is not possible to raise ValueError on all invalid cases, at least without putting everything inside of a big try/except block and raising ValueError from given error. A simple example would be evaluating something like evaluating List(Constant(1))...

isidentical · 2020-05-03T12:05:44Z

@remilapeyre by the way, you can check out this issue to see all possible errors that can be raised from ast.literal_eval https://bugs.python.org/issue39159

remilapeyre · 2020-05-03T14:00:28Z

Oh thanks, I get it now. Indeed there is not much improvement to just check for that. Thanks for the explanantion @isidentical, sorry for the useless noise @brandtbucher.

isidentical

It looks good, but I'd prefer not having 2 same raise statements. Because it might be change in the future and changing both of them seems unreasonable so IMHO it would be cool to create a subfunction called malformed() or invalid() inside of literal_eval and raise from it.

Abstracted calling ValueError in ast.py

brandtbucher

Looks good, thanks!

brandtbucher · 2020-05-05T18:10:33Z

@pablogsal, does this look good to you?

pablogsal · 2020-05-05T18:32:52Z

        if isinstance(node, Constant):
            if type(node.value) in (int, float, complex):
                return node.value
-        raise ValueError('malformed node or string: ' + repr(node))
+        _malformed(node)


Suggestion: I suppose at this point we can invert the check to make the intent a bit more clear:

if not isinstance(node, Constant) or type(node.value) not in (int, float, complex): _raise_malformed_node(node) return node.value

pablogsal · 2020-05-05T18:33:37Z

        node_or_string = parse(node_or_string, mode='eval')
    if isinstance(node_or_string, Expression):
        node_or_string = node_or_string.body
+    def _malformed(node):


Please, call this function with a verb, not a noun. Finding malformed in the middle of the code is a bit surprising compared with report_malformed_node or raise_malformed_exception or something similar.

pablogsal · 2020-05-05T18:37:53Z

        self.assertRaises(ValueError, ast.literal_eval, '3+(0+6j)')
        self.assertRaises(ValueError, ast.literal_eval, '-(3+6j)')

+    def test_literal_eval_malformed(self):


I find this name is too generic. Maybe test_literal_eval_malformed_dict_nodes ?

bedevere-bot · 2020-05-05T18:38:01Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pablogsal · 2020-05-05T18:38:55Z

@pablogsal, does this look good to you?

Thanks for pinging me :)

I left some comments but in general LGTM

@isidentical Could you check again the PR also to see if looks good to you as well?

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

Address requested changes.

into astliteraleval

pablogsal

LGTM

pablogsal · 2020-05-05T19:06:56Z

Thanks, @curtisbucher, for the quick turnaround!

isidentical

Looks great, thanks @curtisbucher

ghost · 2020-05-05T19:41:00Z

Thanks @curtisbucher for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

ghost · 2020-05-05T19:41:12Z

GH-19947 is a backport of this pull request to the 3.8 branch.

ghost · 2020-05-05T19:41:14Z

Sorry, @curtisbucher and @pablogsal, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker c21c51235aa8061da6b0593d6f857f42fd92fd8b 3.7

ghost · 2020-05-05T19:41:38Z

Thanks for the PR @curtisbucher ! 🎉

ghost · 2020-05-05T19:42:24Z

Hummm, seems that the backport to 3.7 failed? @curtisbucher could you follow @miss-islington instructions to do the manual backport? Ping me there so I can merge it

ghost · 2020-05-05T19:44:04Z

Actually....is this something we should backport? This can break some code in 3.8 and 3.7 that was previously working (somehow incorrectly). Not sure if is worth the trouble....

ghost · 2020-05-05T19:44:18Z

@brandtbucher @isidentical opinions?

ghost · 2020-05-05T19:48:43Z

Not sure if is worth the trouble....

Agreed. IMHO it is an improvement rather then a bugfix, so we may skip backporting.

Fix literal_eval and add tests

36eb5b8

the-knights-who-say-ni added the CLA signed label May 2, 2020

bedevere-bot added the awaiting review label May 2, 2020

remilapeyre suggested changes May 2, 2020

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 2, 2020

brandtbucher added type-bug An unexpected behavior, bug, or error needs backport to 3.7 labels May 2, 2020

brandtbucher requested changes May 2, 2020

View reviewed changes

isidentical reviewed May 3, 2020

View reviewed changes

Update ast.py

2c72460

Abstracted calling ValueError in ast.py

brandtbucher approved these changes May 4, 2020

View reviewed changes

brandtbucher requested a review from pablogsal May 4, 2020 03:31

pablogsal requested changes May 5, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels May 5, 2020

curtisbucher and others added 3 commits May 5, 2020 11:53

Update Misc/NEWS.d/next/Library/2020-05-02-14-24-48.bpo-40355.xTujaB.rst

3e68c2e

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

Update ast.py and test_ast.py

2427ed2

Address requested changes.

Merge branch 'astliteraleval' of https://github.com/curtisbucher/cpython

bc9274f

into astliteraleval

pablogsal approved these changes May 5, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels May 5, 2020

isidentical approved these changes May 5, 2020

View reviewed changes

pablogsal merged commit c21c512 into python:master May 5, 2020

bedevere-bot removed awaiting merge needs backport to 3.8 labels May 5, 2020

miss-islington assigned pablogsal May 5, 2020

pablogsal removed the needs backport to 3.7 label May 5, 2020

curtisbucher deleted the astliteraleval branch May 5, 2020 19:52

Conversation

curtisbucher commented May 2, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remilapeyre left a comment

Choose a reason for hiding this comment

Uh oh!

brandtbucher commented May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandtbucher May 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isidentical commented May 3, 2020

Uh oh!

remilapeyre commented May 3, 2020

Uh oh!

isidentical commented May 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isidentical commented May 3, 2020

Uh oh!

remilapeyre commented May 3, 2020

Uh oh!

isidentical left a comment

Choose a reason for hiding this comment

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

brandtbucher commented May 5, 2020

Uh oh!

pablogsal May 5, 2020

Choose a reason for hiding this comment

Uh oh!

pablogsal May 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pablogsal May 5, 2020

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 5, 2020

Uh oh!

pablogsal commented May 5, 2020

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

pablogsal commented May 5, 2020

Uh oh!

isidentical left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented May 5, 2020

Uh oh!

ghost commented May 5, 2020

Uh oh!

ghost commented May 5, 2020

Uh oh!

ghost commented May 5, 2020

Uh oh!

ghost commented May 5, 2020

Uh oh!

ghost commented May 5, 2020

Uh oh!

ghost commented May 5, 2020

Uh oh!

ghost commented May 5, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

curtisbucher commented May 2, 2020 •

edited by bedevere-bot

Loading

brandtbucher commented May 2, 2020 •

edited

Loading

brandtbucher May 2, 2020 •

edited

Loading

isidentical commented May 3, 2020 •

edited

Loading