homepage

Following code crashes. (See issue4170)
trunk/Modules/cPickle.c (save() or save_reduce()) needs more checks of
returned value from __reduce__.
class C(object):
 def __reduce__(self):
 return C, (), None, None, [] # 5th item is not an iterator
class D(object):
 def __reduce__(self):
 return D, (), None, [], None # 4th item is not an iterator
import sys
if sys.version_info[0] == 3:
 import pickle
else:
 import cPickle as pickle
pickle.dumps(C()) # crash
pickle.dumps(D()) # crash

save() and save_reduce() seems to check same thing.
- tuple size
- 2nd item is tuple or not
Is this duplicated or intended? If we adds check for 4th/5th item,
should them be in both functions?

The attached patch adds a test and corrects the crash.

I think the patch is fine.

Committed r67048 (trunk), r67056 (2.5), r67053 (2.6), r67059 (py3k).

Thank you, Amaury, for fixing this. However, you forgot to also patch
the Python implementation of pickle, which makes the following test fail:
======================================================================
FAIL: test_reduce_bad_iterator (__main__.PyPicklerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/home/alex/src/python.org/py3k/Lib/test/pickletester.py", line
892, in test_reduce_bad_iterator
 self.assertRaises(pickle.PickleError, self.dumps, C(), proto)
AssertionError: PickleError not raised by dumps
Also, I am not sure if moving the type and length check of the reduce
value into save_reduce() was a good idea. In pickle.py, the check must 
be done before calling save_reduce(), since the method is called with
the star-syntax "self.save_reduce(obj=obj, *rv)". In _pickle, there's no
such requirement; however I don't like the idea of adding needless
differences.

>the Python implementation of pickle
This was out of my mind. I hope fix_pickle_consistency_on_py3k.patch can
fix the buildbot error. ;-)

Iterators only need to provide a __next__() method. So, you don't have
to check __iter__.
Other than that, the patch looks good.

Thank you for review.
>Iterators only need to provide a __next__() method. So, you don't have
>to check __iter__.
Hmm, but python document says,
(http://docs.python.org/library/stdtypes.html#typeiter)
>The iterator objects themselves are required to support the
>following two methods, which together form the iterator protocol:
Is this false information?

I corrected the test in the py3k branch.
I don't know why I did not make it similar to the one in trunk; I guess 
I ran the test suite on the wrong directory... Sorry for the 
inconvenience.
Regarding the consistency between python and C, I don't think it is 
necessary to be so strict. If the python version can allow a weaker 
version of the "iterator" concept, let be it.
OTOH, in 3.0 the C version was rewritten to be quasi identical to the 
Python one, so I do understand your desire to have similar code.

Hirokazu Yamamoto wrote
> Hmm, but python document says,
> (http://docs.python.org/library/stdtypes.html#typeiter)
>
> >The iterator objects themselves are required to support the
> >following two methods, which together form the iterator protocol:
>
> Is this false information?
Oh, you are right. I got confused by looking at the implementation of
PyIter_Check(), which only verifies that tp_next is not NULL. Anyway, 
_batch_appends() and _batch_setitems() in pickle.py calls iter() on the
iterators; so, the iterators must provide __iter__ too.
Personally, I wouldn't bother about checking for __iter__ to avoid
adding a new method to Pickler and also to save a few hasattr() calls. 
But I have to admit, it is really just nitpicking at this point.

Amaury Forgeot d'Arc wrote:
> Regarding the consistency between python and C, I don't think it is 
> necessary to be so strict. If the python version can allow a weaker 
> version of the "iterator" concept, let be it.
I guess you're right. However, my only fear is that people will write
code for pickle.py and then, when they run it on _pickle, their code
will fail due to the stricter requirements. In addition, I think having
the same requirements make it easier to write tests for both
implementations.