Pickle `APPENDS` and `ADDITEMS` missing check

[Legoclones]

Bug report

Bug description:

A small inconsistency in behavior between pickle and _pickle exists for the ADDITEMS and APPENDS opcodes. Both opcodes are meant to add items to a set or list by popping off all objects delimited by the MARK object. Then, an add()/extend()/append() attribute function is called to add the new items to the object.

In the C accelerator, the number of items to be added is explicitly checked in both opcodes, and if it's 0 it returns early.

cpython/Modules/_pickle.c

Lines 6638 to 6639 in f079979

	if (len == mark) /* nothing to do */
	return 0;

cpython/Modules/_pickle.c

Lines 6494 to 6495 in f079979

	if (len == x) /* nothing to do */
	return 0;

However in the Python version, there is no check for 0 items.

cpython/Lib/pickle.py

Lines 1818 to 1826 in f079979

	def load_additems(self):
	items = self.pop_mark()
	set_obj = self.stack[-1]
	if isinstance(set_obj, set):
	set_obj.update(items)
	else:
	add = set_obj.add
	for item in items:
	add(item)

cpython/Lib/pickle.py

Lines 1785 to 1800 in f079979

	def load_appends(self):
	items = self.pop_mark()
	list_obj = self.stack[-1]
	try:
	extend = list_obj.extend
	except AttributeError:
	pass
	else:
	extend(items)
	return
	# Even if the PEP 307 requires extend() and append() methods,
	# fall back on append() if the object has no extend() method
	# for backward compatibility.
	append = list_obj.append
	for item in items:
	append(item)

This normally wouldn't cause any inconsistencies unless the item on the top of the stack (ie the list or set being appended to) isn't actually a list or set. For example, if we put an integer on the stack instead of a list and used the APPENDS opcode to add 0 items, it will error in pickle since the integer doesn't have an append() attribute, but it will just return in _pickle due to the check.

The following payloads demonstrate the inconsistencies:

payload:      b'K\x01(e.'

pickle:       FAILURE 'int' object has no attribute 'append'
_pickle.c:    1
pickletools:  
    0: K    BININT1    1
    2: (    MARK
    3: e        APPENDS    (MARK at 2)
    4: .    STOP
highest protocol among opcodes = 1

payload:      b'K\x01(\x90.'

pickle:       FAILURE 'int' object has no attribute 'add'
_pickle.c:    1
pickletools:  
    0: K    BININT1    1
    2: (    MARK
    3: \x90     ADDITEMS   (MARK at 2)
    4: .    STOP
highest protocol among opcodes = 4

This can be easily mitigated by adding the same check for 0 items to the Python version of the pickle module for both opcodes.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

• gh-135573: Add a check for 0 items to APPENDS and ADDITEMS opcodes #135574

[serhiy-storchaka]

The check for 0 items in the C implementation is an optimization. The check for 0 items in the Python implementation would not be so cheap. It would add an overhead. For very large lists and sets, only 1 of 1000 APPENDS and ADDITEMS operations will have 0 items.

Other drawback is that an error in invalid pickle is not detected.

We could remove the check for 0 items, but then we need to change the pickling code so that it does not produce APPENDS and ADDITEMS with 0 items. This would also result in smaller pickle data, but it needs more complex code.