classification
Title: Pickler.dump from a badly initialized Pickler segfaults
Type: crash Stage:
Components: Versions: Python 3.0
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: alexandre.vassalotti Nosy List: ajaksu2, alexandre.vassalotti, amaury.forgeotdarc, barry, christian.heimes, jcea
Priority: release blocker Keywords: patch

Created on 2008-08-24 20:26 by ajaksu2, last changed 2008-10-17 20:16 by amaury.forgeotdarc. This issue is now closed.

Files
File name Uploaded Description Edit
_pickle.c alexandre.vassalotti, 2008-10-04 22:54
issue3664_fix.diff alexandre.vassalotti, 2008-10-17 05:10
Messages (11)
msg71860 - (view) Author: Daniel Diniz (ajaksu2) Date: 2008-08-24 20:26
This script segfaults:
##
import _pickle
obj = _pickle.Pickler(open("/bin/ls")) #can be open(__file__) for scripts
try: obj.__init__('pouet', 87)
except Exception as err: pass

obj.dump(0)
###


[Switching to Thread -1210775360 (LWP 19096)]
0xb79fbf91 in pickler_write (self=0xb7a2fe4c, s=0xbff441a1 "...", n=2)
    at /home/ajaksu/py3k/Modules/_pickle.c:442
442                 memcpy(self->write_buf + self->buf_size, s, n);
(gdb) backtrace
#0  0xb79fbf91 in pickler_write (self=0xb7a2fe4c, s=0xbff441a1 "...",
    n=2) at /home/ajaksu/py3k/Modules/_pickle.c:442
#1  0xb7a00a8c in dump (self=0xb7a2fe4c, obj=0x81fbd78) at
/home/ajaksu/py3k/Modules/_pickle.c:2288
#2  0xb7a00bb8 in Pickler_dump (self=0xb7a2fe4c, args=0xb7b30034) at
/home/ajaksu/py3k/Modules/_pickle.c:2328
#3  0x081626b1 in PyCFunction_Call (func=0xb796c3ec, arg=0xb7b30034,
kw=0x0) at Objects/methodobject.c:81
#4  0x080b378f in call_function (pp_stack=0xbff442e4, oparg=1) at
Python/ceval.c:3403
#5  0x080ae8d2 in PyEval_EvalFrameEx (f=0x829bafc, throwflag=0) at
Python/ceval.c:2205
#6  0x080b1c24 in PyEval_EvalCodeEx (co=0xb7acf2c8, globals=0xb7a9a8f4,
locals=0xb7a9a8f4, args=0x0, argcount=0, kws=0x0,
    kwcount=0, defs=0x0, defcount=0, kwdefs=0x0, closure=0x0) at
Python/ceval.c:2840

Found using Fusil.
msg71863 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-08-24 20:44
pickler_write() has no check for self->write_buf == NULL

Suggested patch:
===================================================================
--- Modules/_pickle.c   (Revision 66010)
+++ Modules/_pickle.c   (Arbeitskopie)
@@ -421,6 +421,10 @@
 {
     PyObject *data, *result;

+    if (self->write_buf == NULL) {
+        PyErr_SetString(PyExc_SystemError, "Invalid write buffer");
+        return -1;
+    }
     if (s == NULL) {
         if (!(self->buf_size))
             return 0;
msg71938 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2008-08-25 15:25
Oh, that's nasty. Recalling __init__ with bad arguments breaks the
internal invariants as it clears the Pickler's content before parsing
the arguments. I suspect that Unpickler is vulnerable too.

Adding a NULL check in pickler_write will only fix this particular
example. I could probably find another crash example that doesn't use
pickler_write.
msg71942 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2008-08-25 16:00
Unpickler looks safe as Unpickler_load() checks if Unpickler was
properly initialized. And only Pickler_dump is vulnerable right now (new
methods, if any, exposed for issue3385 will have to take into account
this vulnerability).
msg74164 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2008-10-02 14:36
I will try to find time next weekend to fix this (and other pickle 
blockers).
msg74327 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2008-10-04 22:54
Here's the fix. The added check in Pickler_dump should prevent any
segfaults due to __init__() errors. 

I also added the check proposed by Christian as a safe-guard in case a
core developer adds a new method that doesn't check if the object was
properly initialized.
msg74888 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2008-10-17 01:34
Rather than attach a full _pickle.c file, please generate a unified diff
with just your changes.  The patch should include a test for the
crashing condition.  If you can upload that I'll try to accept it for
rc3.  Deferring for now.
msg74896 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2008-10-17 05:10
Oops. I must have been quite tired when I submitted that.

Here's the patch for the fix and the test case.
msg74898 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-10-17 07:48
The patch is fine.
msg74902 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2008-10-17 11:52
Amaury, please apply the patch and close the issue.  Thanks!
msg74936 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-10-17 20:16
Committed r66963.
History
Date User Action Args
2008-10-17 20:16:14amaury.forgeotdarcsetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg74936
2008-10-17 11:52:42barrysetpriority: deferred blocker -> release blocker
messages: + msg74902
2008-10-17 07:49:00amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
resolution: accepted
messages: + msg74898
2008-10-17 05:10:11alexandre.vassalottisetfiles: + issue3664_fix.diff
keywords: + patch
messages: + msg74896
2008-10-17 01:34:45barrysetpriority: release blocker -> deferred blocker
nosy: + barry
messages: + msg74888
2008-10-04 22:54:16alexandre.vassalottisetfiles: + _pickle.c
messages: + msg74327
2008-10-02 20:13:59jceasetnosy: + jcea
2008-10-02 14:36:27alexandre.vassalottisetmessages: + msg74164
2008-10-02 14:36:15alexandre.vassalottisetmessages: - msg74163
2008-10-02 14:35:52alexandre.vassalottisetmessages: + msg74163
2008-10-02 12:52:55barrysetpriority: deferred blocker -> release blocker
2008-09-26 22:17:25barrysetpriority: release blocker -> deferred blocker
2008-09-18 05:41:57barrysetpriority: deferred blocker -> release blocker
2008-09-04 01:15:19benjamin.petersonsetpriority: release blocker -> deferred blocker
2008-08-25 16:00:03alexandre.vassalottisetmessages: + msg71942
2008-08-25 15:25:45alexandre.vassalottisetmessages: + msg71938
2008-08-24 20:44:11christian.heimessetnosy: + christian.heimes
messages: + msg71863
2008-08-24 20:27:45benjamin.petersonsetpriority: release blocker
assignee: alexandre.vassalotti
nosy: + alexandre.vassalotti
2008-08-24 20:26:34ajaksu2create