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, serhiy.storchaka
Priority: high Keywords: patch

Created on 2009-09-24 11:21 by haypo, last changed 2017-09-19 13:06 by haypo. 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 (15)
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!
msg302513 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-09-19 11:10
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.
msg302522 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-09-19 13:06
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.)
History
Date User Action Args
2017-09-19 13:06:45hayposetmessages: + msg302522
2017-09-19 11:10:03serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg302513
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