[issue6986] _json crash on scanner/encoder initialization error

Serhiy Storchaka added the comment: New changeset 7c2d97827fd2ccd72ab7a98427f87fcfe3891644 by Serhiy Storchaka (Oren Milman) in branch 'master': bpo-6986: Add a comment to clarify a test of _json.make_encoder(). (GH-3789)

[issue6986] _json crash on scanner/encoder initialization error

Change by Oren Milman : -- pull_requests: +3775 ___ Python tracker ___ ___ Python-bugs-list

[issue6986] _json crash on scanner/encoder initialization error

Serhiy Storchaka added the comment: It would be nice. -- ___ Python tracker ___

[issue6986] _json crash on scanner/encoder initialization error

Oren Milman added the comment: I would be happy to open such a PR, if you don't mind. -- nosy: +Oren Milman ___ Python tracker ___

[issue6986] _json crash on scanner/encoder initialization error

STINNER Victor added the comment: Oh wow, this issue was fixed 8 years ago and it's now closed! Please open a write issue, or maybe even propose directly a PR? (If you do, I will review it.) -- ___ Python tracker

[issue6986] _json crash on scanner/encoder initialization error

Serhiy Storchaka added the comment: Could you please add a comment for test_make_encoder Victor? Invalid arguments are passed to c_make_encoder, but it raises a TypeError not because wrong types of argument, but because the number of arguments is insufficient. At first glance it looks as an

[issue6986] _json crash on scanner/encoder initialization error

STINNER Victor victor.stin...@haypocalc.com added the comment: I wrote new patches to: - implement the alternate method - add unit tests - and finally to move the tests in the right file Is the patch now correct? Can you apply it? It doesn't still crash Python trunk and it generates a lot of

[issue6986] _json crash on scanner/encoder initialization error

Antoine Pitrou pit...@free.fr added the comment: The patch looks good to me. -- priority: - high resolution: - accepted stage: - commit review type: - crash versions: +Python 2.7, Python 3.1, Python 3.2 ___ Python tracker rep...@bugs.python.org

[issue6986] _json crash on scanner/encoder initialization error

Antoine Pitrou pit...@free.fr added the comment: This has been committed in r76708, r76710, r76711. Thank you! -- resolution: accepted - fixed stage: commit review - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org

[issue6986] _json crash on scanner/encoder initialization error

STINNER Victor victor.stin...@haypocalc.com added the comment: New version of my patch: * don't create file in Lib/json/tests/: add new tests in Lib/json/tests/test_speedups.py as asked by pitrou * use json.scanner.c_make_scanner and json.encoder.c_make_encoder in the test, instead of using

[issue6986] _json crash on scanner/encoder initialization error

Changes by STINNER Victor victor.stin...@haypocalc.com: Removed file: http://bugs.python.org/file15291/json-crash-2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6986 ___

[issue6986] _json crash on scanner/encoder initialization error

STINNER Victor victor.stin...@haypocalc.com added the comment: Oops, you forget json-crash-2.patch, the patch was completly wrong. Check the json-crash-3.patch. -- Added file: http://bugs.python.org/file15292/json-crash-3.patch ___ Python tracker

[issue6986] _json crash on scanner/encoder initialization error

Antoine Pitrou pit...@free.fr added the comment: The tests could go in Lib/json/tests/test_speedups.py. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6986 ___

[issue6986] _json crash on scanner/encoder initialization error

STINNER Victor victor.stin...@haypocalc.com added the comment: pitrou Unit tests are definitely desireable! done pitou I would also like the alternate approach fix, pitou it would probably be cleaner. done. See the new patch. -- Added file:

[issue6986] _json crash on scanner/encoder initialization error

Antoine Pitrou pit...@free.fr added the comment: There is actually a Lib/json/tests/ directory contain all JSON tests, you should add yours there. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6986

[issue6986] _json crash on scanner/encoder initialization error

STINNER Victor victor.stin...@haypocalc.com added the comment: pitrou There is actually a Lib/json/tests/ directory contain all pitrou JSON tests, you should add yours there. My patch creates the file Lib/json/tests/test__json.py. Do you mean that I should add the new tests to an existing

[issue6986] _json crash on scanner/encoder initialization error

Antoine Pitrou pit...@free.fr added the comment: Ah, my bad, I thought it created the file in Lib/test. The tests probably need to be skipped if _json can't be imported, though. (the _json accelerators are just an implementation detail) -- ___ Python

[issue6986] _json crash on scanner/encoder initialization error

New submission from STINNER Victor victor.stin...@haypocalc.com: scanner_init() and encoder_init() don't manage errors correctly. scanner_init() gets context.encoding argument without checking context type, nor GetAttrString() error. It should check for NULL result... which is done in the same

[issue6986] _json crash on scanner/encoder initialization error

Changes by STINNER Victor victor.stin...@haypocalc.com: Added file: http://bugs.python.org/file14965/_json_scanner_init.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6986 ___

[issue6986] _json crash on scanner/encoder initialization error

STINNER Victor victor.stin...@haypocalc.com added the comment: About _json_encoder_init.patch, an alternative patch is to write arguments references (addresses) in local variables, and only copy them on success. Something like: PyObject *arg; if (!PyTuple_ParseArgs(..., arg)) return NULL;

[issue6986] _json crash on scanner/encoder initialization error

Changes by Benjamin Peterson benja...@python.org: -- assignee: - bob.ippolito nosy: +bob.ippolito ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue6986 ___

[issue6986] _json crash on scanner/encoder initialization error

Antoine Pitrou pit...@free.fr added the comment: Unit tests are definitely desireable! I would also like the alternate approach fix, it would probably be cleaner. -- nosy: +pitrou ___ Python tracker rep...@bugs.python.org