_json crash on scanner/encoder initialization error

[vstinner]

BPO	6986
Nosy	@etrepum, @pitrou, @vstinner, @serhiy-storchaka, @orenmn
PRs	bpo-6986: Add a comment to clarify a test of _json.make_encoder() #3789
Files	_json_encoder_init.patch _json_scanner_init.patch json-crash.patch json-crash-3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/etrepum'
closed_at = <Date 2009-12-08.16:09:58.926>
created_at = <Date 2009-09-24.11:21:05.754>
labels = ['type-crash']
title = '_json crash on scanner/encoder initialization error'
updated_at = <Date 2018-03-26.15:04:44.469>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2018-03-26.15:04:44.469>
actor = 'serhiy.storchaka'
assignee = 'bob.ippolito'
closed = True
closed_date = <Date 2009-12-08.16:09:58.926>
closer = 'pitrou'
components = []
creation = <Date 2009-09-24.11:21:05.754>
creator = 'vstinner'
dependencies = []
files = ['14964', '14965', '15190', '15292']
hgrepos = []
issue_num = 6986
keywords = ['patch']
message_count = 18.0
messages = ['93068', '93069', '93077', '94396', '94397', '94398', '94399', '94429', '95052', '95054', '96125', '96131', '96134', '302513', '302522', '303129', '303139', '314457']
nosy_count = 5.0
nosy_names = ['bob.ippolito', 'pitrou', 'vstinner', 'serhiy.storchaka', 'Oren Milman']
pr_nums = ['3789']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue6986'
versions = ['Python 3.1', 'Python 2.7', 'Python 3.2']

[vstinner]

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 function for other attributes (strict,
object_hook, object_pairs_hook, parse_float, parse_int, parse_constant).

Example to reproduce the crash:
import _json
_json.make_scanner(1)

encoder_init() copies a refence (for each argument) without incrementing
the reference counter. And then encoder_clear() decrements the
reference, counter, which may crash Python.

Example to reproduce the crash:
import _json
_json.make_encoder(
(False, True),
-826484143518891896,
-56.0,
"a",
)
# do anything creating/destroying new objects
" abc ".strip()
len(" xef ".strip())

Attached patches for the crashes.

[vstinner]

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;
  Py_INCREF(arg);
  self->arg = arg;

I prefered to write a shorter patch.

Tell me if you prefer the alternative fix, and if you would like unit tests.

[pitrou]

Unit tests are definitely desireable!
I would also like the "alternate approach" fix, it would probably be
cleaner.

[vstinner]

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.

[pitrou]

There is actually a Lib/json/tests/ directory contain all JSON tests,
you should add yours there.

[vstinner]

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 file? Which one?

[pitrou]

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)

[pitrou]

The tests could go in Lib/json/tests/test_speedups.py.

[vstinner]

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 directly the _json module

[vstinner]

Oops, you forget json-crash-2.patch, the patch was completly wrong.
Check the json-crash-3.patch.

[vstinner]

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 noise in my fuzzer :-)

[pitrou]

The patch looks good to me.

[pitrou]

This has been committed in r76708, r76710, r76711. Thank you!