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.

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.

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

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.

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

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?

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)

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

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

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

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 :-)

The patch looks good to me.

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

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 error in the test. Only reading this issue give a hint that perhaps this is intentional. But there is no reference to this issue in the test.

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.)

I would be happy to open such a PR, if you don't mind.

It would be nice.

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