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
• gh-135573: Make pickled lists, sets and dicts a tiny bit smaller #144162
• gh-135573: Add tests for pickle opcodes with wrong types #144950

[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.

[serhiy-storchaka]

It does not actually complicates the code, all bits are already here, they just need some rearranging. And the Python implementation already guarantees this.

[serhiy-storchaka]

APPENDS and ADDITEMS with empty items were very rare, and now they will be never produced in normal circumstances (unless the collection was modified during pickling). #144950 ensures that an error is raised even for empty items in both implementations and adds more tests for similar cases. Although there is no validation for SETITEMS with empty items. Weird, but it's not worth it.