Issue 25262: Issues with BINUNICODE8 and BINBYTES8 opcodes in pickle

There are issues with BINUNICODE8 and BINBYTES8 opcodes in pickle.

1. Unpickling BINBYTES8 is not implemented in Python implementation.

2. Highest 32 bits of 64-bit size are silently ignored in C implementation on 32-bit platforms.

3. There are no tests for BINUNICODE8 and BINBYTES8.

Proposed patch fixes these issues.

To access an array item, the type of the index variable must be size_t (or a type with the same size), otherwise the compiler produce less efficient machine code:
http://www.viva64.com/en/a/0050/

=> please keep Py_ssize_t type for i

(I didn't check for the specific case of Py_ssize_t: it's signed. Does GCC emit the most efficient machine code for it?)

diff -r 16c8278c03f6 Modules/_pickle.c
--- a/Modules/_pickle.c
+++ b/Modules/_pickle.c
@@ -4606,10 +4606,17 @@ static Py_ssize_t
 calc_binsize(char *bytes, int nbytes)
 {
     unsigned char *s = (unsigned char *)bytes;
-    Py_ssize_t i;
+    int i;
     size_t x = 0;
 
-    for (i = 0; i < nbytes && (size_t)i < sizeof(size_t); i++) {
+    if (nbytes > (int)sizeof(size_t)) {
+        for (i = (int)sizeof(size_t); i < nbytes; i++) {
+            if (s[i])
+                return -1;
+        }
+        nbytes = (int)sizeof(size_t);
+    }

Please add a comment here to explain that the first loop check for integer overflow, it's not obvious at the first read.

Does the Python implementation of pickle produce BINBYTES8? If not: why not?

Note: the patch is probably based on a private Mercurial revision, so it didn't get the [Review] button.

> To access an array item, the type of the index variable must be size_t (or a
> type with the same size), otherwise the compiler produce less efficient
> machine code: http://www.viva64.com/en/a/0050/
> 
> => please keep Py_ssize_t type for i

i is small integer between 0 and 8. I don't think that this article is 
related. Py_ssize_t is signed, and i is compared with int in a loop. And isn't 
Viva64 work only with Visual C++?  At least I found statements that can be not 
correct on platforms not supported by Microsoft.

> Please add a comment here to explain that the first loop check for integer
> overflow, it's not obvious at the first read.

Done.

> Does the Python implementation of pickle produce BINBYTES8? If not: why not?

Yes, it produces BINBYTES8 on 64-bit platform.

> Note: the patch is probably based on a private Mercurial revision, so it
> didn't get the [Review] button.

Yes, I had made some refactoring for unpickling tests and forgot to push they. 
Here is rebased patch.

New changeset d4f8316d0860 by Serhiy Storchaka in branch '3.4':
Issue #25262. Added support for BINBYTES8 opcode in Python implementation of
https://hg.python.org/cpython/rev/d4f8316d0860

New changeset da9ad20dd470 by Serhiy Storchaka in branch '3.5':
Issue #25262. Added support for BINBYTES8 opcode in Python implementation of
https://hg.python.org/cpython/rev/da9ad20dd470

New changeset 8de1967edfdb by Serhiy Storchaka in branch 'default':
Issue #25262. Added support for BINBYTES8 opcode in Python implementation of
https://hg.python.org/cpython/rev/8de1967edfdb

Thank you Victor and Antoine for your reviews.

stage: patch review -> resolved

messages: + msg251856