classification
Title: _json crash on scanner/encoder initialization error
Type: crash Stage: resolved
Components: Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: bob.ippolito Nosy List: bob.ippolito, haypo, pitrou
Priority: high Keywords: patch

Created on 2009-09-24 11:21 by haypo, last changed 2009-12-08 16:09 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
_json_encoder_init.patch haypo, 2009-09-24 11:21
_json_scanner_init.patch haypo, 2009-09-24 11:21
json-crash.patch haypo, 2009-10-23 22:22
json-crash-3.patch haypo, 2009-11-09 01:41
Messages (13)
msg93068 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-09-24 11:21
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.
msg93069 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-09-24 11:23
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.
msg93077 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-09-24 14:43
Unit tests are definitely desireable!
I would also like the "alternate approach" fix, it would probably be
cleaner.
msg94396 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-10-23 22:22
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.
msg94397 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-10-23 23:31
There is actually a Lib/json/tests/ directory contain all JSON tests,
you should add yours there.
msg94398 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-10-23 23:33
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?
msg94399 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-10-23 23:38
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)
msg94429 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-10-24 16:27
The tests could go in Lib/json/tests/test_speedups.py.
msg95052 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-11-09 01:35
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
msg95054 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-11-09 01:41
Oops, you forget json-crash-2.patch, the patch was completly wrong.
Check the json-crash-3.patch.
msg96125 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2009-12-08 11:27
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 :-)
msg96131 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-12-08 15:35
The patch looks good to me.
msg96134 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-12-08 16:09
This has been committed in r76708, r76710, r76711. Thank you!
History
Date User Action Args
2009-12-08 16:09:58pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg96134

stage: commit review -> resolved
2009-12-08 15:35:57pitrousetpriority: high
versions: + Python 3.1, Python 2.7, Python 3.2
type: crash
messages: + msg96131

resolution: accepted
stage: commit review
2009-12-08 11:27:36hayposetmessages: + msg96125
2009-11-09 01:41:11hayposetfiles: + json-crash-3.patch

messages: + msg95054
2009-11-09 01:40:09hayposetfiles: - json-crash-2.patch
2009-11-09 01:35:32hayposetfiles: + json-crash-2.patch

messages: + msg95052
2009-10-24 16:27:38pitrousetmessages: + msg94429
2009-10-23 23:38:14pitrousetmessages: + msg94399
2009-10-23 23:33:58hayposetmessages: + msg94398
2009-10-23 23:31:06pitrousetmessages: + msg94397
2009-10-23 22:22:18hayposetfiles: + json-crash.patch

messages: + msg94396
2009-09-24 14:43:00pitrousetnosy: + pitrou
messages: + msg93077
2009-09-24 14:19:20benjamin.petersonsetassignee: bob.ippolito

nosy: + bob.ippolito
2009-09-24 11:23:27hayposetmessages: + msg93069
2009-09-24 11:21:19hayposetfiles: + _json_scanner_init.patch
2009-09-24 11:21:05haypocreate