Issue 3664: Pickler.dump from a badly initialized Pickler segfaults (original) (raw)
Created on 2008-08-24 20:26 by ajaksu2, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (11)
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.
Author: Christian Heimes (christian.heimes) * 
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;
Author: Alexandre Vassalotti (alexandre.vassalotti) *
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.
Author: Alexandre Vassalotti (alexandre.vassalotti) *
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 will have to take into account this vulnerability).
Author: Alexandre Vassalotti (alexandre.vassalotti) *
Date: 2008-10-02 14:36
I will try to find time next weekend to fix this (and other pickle blockers).
Author: Alexandre Vassalotti (alexandre.vassalotti) *
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.
Author: Barry A. Warsaw (barry) *
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.
Author: Alexandre Vassalotti (alexandre.vassalotti) *
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.
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *
Date: 2008-10-17 07:48
The patch is fine.
Author: Barry A. Warsaw (barry) *
Date: 2008-10-17 11:52
Amaury, please apply the patch and close the issue. Thanks!
Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *
Date: 2008-10-17 20:16
Committed r66963.
History
Date
User
Action
Args
2022-04-11 14:56:38
admin
set
github: 47914
2008-10-17 20:16:14
amaury.forgeotdarc
set
status: open -> closed
resolution: accepted -> fixed
messages: +
2008-10-17 11:52:42
barry
set
priority: deferred blocker -> release blocker
messages: +
2008-10-17 07:49:00
amaury.forgeotdarc
set
nosy: + amaury.forgeotdarc
resolution: accepted
messages: +
2008-10-17 05:10:11
alexandre.vassalotti
set
files: + issue3664_fix.diff
keywords: + patch
messages: +
2008-10-17 01:34:45
barry
set
priority: release blocker -> deferred blocker
nosy: + barry
messages: +
2008-10-04 22:54:16
alexandre.vassalotti
set
files: + _pickle.c
messages: +
2008-10-02 20:13:59
jcea
set
nosy: + jcea
2008-10-02 14:36:27
alexandre.vassalotti
set
messages: +
2008-10-02 14:36:15
alexandre.vassalotti
set
messages: -
2008-10-02 14:35:52
alexandre.vassalotti
set
messages: +
2008-10-02 12:52:55
barry
set
priority: deferred blocker -> release blocker
2008-09-26 22:17:25
barry
set
priority: release blocker -> deferred blocker
2008-09-18 05:41:57
barry
set
priority: deferred blocker -> release blocker
2008-09-04 01:15:19
benjamin.peterson
set
priority: release blocker -> deferred blocker
2008-08-25 16:00:03
alexandre.vassalotti
set
messages: +
2008-08-25 15:25:45
alexandre.vassalotti
set
messages: +
2008-08-24 20:44:11
christian.heimes
set
nosy: + christian.heimes
messages: +
2008-08-24 20:27:45
benjamin.peterson
set
priority: release blocker
assignee: alexandre.vassalotti
nosy: + alexandre.vassalotti
2008-08-24 20:26:34
ajaksu2
create