Pickle `NEWOBJ`/`NEWOBJ_EX` opcodes don't type check `arg`

[Legoclones]

Bug report

Bug description:

Both the NEWOBJ and NEWOBJ_EX opcodes build a new class instance using user-provided arguments (arg). Both the pickletools code comments and the C accelerator _pickle module enforce the type of arg to be a tuple, but Python pickle does not.

cpython/Lib/pickle.py

Lines 1624 to 1636 in 4c15505

	def load_newobj(self):
	args = self.stack.pop()
	cls = self.stack.pop()
	obj = cls.__new__(cls, *args)
	self.append(obj)
	dispatch[NEWOBJ[0]] = load_newobj
	def load_newobj_ex(self):
	kwargs = self.stack.pop()
	args = self.stack.pop()
	cls = self.stack.pop()
	obj = cls.__new__(cls, *args, **kwargs)
	self.append(obj)

cpython/Modules/_pickle.c

Line 6019 in 4c15505

if (!PyTuple_Check(args)) {

Therefore, if args is an iterable that's not a tuple (like a list), C _pickle will throw an error, but Python pickle will deserialize just fine.

Here are payloads for both opcodes that demonstrate this:

payload:      b'c__main__\nX\n]\x81.'

pickle:       <__main__.X object at 0x7f7411fbd6a0>
_pickle.c:    FAILURE NEWOBJ args argument must be a tuple, not list
pickletools:  
    0: c    GLOBAL     '__main__ X'
    12: ]    EMPTY_LIST
    13: \x81 NEWOBJ
    14: .    STOP
highest protocol among opcodes = 2

payload:      b'c__main__\nX\n]}\x92.'

pickle:       <__main__.X object at 0x7f0a83fd19d0>
_pickle.c:    FAILURE NEWOBJ_EX args argument must be a tuple, not list
pickletools:  
    0: c    GLOBAL     '__main__ X'
    12: ]    EMPTY_LIST
    13: }    EMPTY_DICT
    14: \x92 NEWOBJ_EX
    15: .    STOP
highest protocol among opcodes = 4

I think the easiest solution would be to just add an if not isinstance(args, tuple) check to both Python opcode load functions.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

[serhiy-storchaka]

The isinstance() check would add an overhead.

The code works correctly for valid pickle. It does not detect some errors in invalid pickle, but this is not a bug, it is just a lack of feature. I am not sure that the benefit of the detection is worth the cost.

[picnixz]

I also don't think we should change this specifically. Invalid pickle would only be deserialized by the pure python one, but it's only if we have something supporting unpacking through *. There are other places where the pure Python module is less restrictive than the C module for invalid data (one example is the XML module or OrderedDict where sometimes exceptions are fired by the C implementation but not by the Python one because we apply "garbage in -> garbage out"). Here, it's just that pure Python pickle has more features but that "additional" feature is not part of the contract.

Note that the docs for __getnewargs_ex__ say that we must return a tuple and a dict. So in a sense, we could expect that those are correct. The reason why we need the check in C is because we are calling tp_new which requires tuples and dicts (not just unpackable sequences).