Don't use an ill-defined word such as "safe" in the name. Just describe what it does. chunked_file_read() makes more sense. Same with SAFE_BUF_SIZE just call it _READ_BUF_SIZE (with an underscore prefix so it isn't considered a public API).

Note that there is _safe_read in http.client, although ironically it is vulnerable to this kind of issue.

I afraid that name like chunked_file_read could be misleading, as the data is not read by fixed size chunks. I guess this is what leads you to misunderstanding about the O(N^2) behavior.

where does (1 << 17) come from? that feels like it should be a constant or at least deserves a command as to what the significance of this value is and why this limit exists.

It is just an arbitrary constant to make the existing tests pass. It should be larger than 2**16, otherwise we will need to make input data for two tests much larger. I do not think that the real code needs more than 0 or 1. I have no good name for this. The Python code does not need this, as it does not have such limitation and is not vulnerable to this issue (but it is less efficient).

This is the part of this PR that I have the greatest doubts. It can make the unpickler to reject an unusual data that is accepted now, although such data never produced by the pickler (it can still be unpickled with the Python implementation).

I can make the C code more similar to the Python code (as a fallback). But this will add a lot of complexity, and I do not know whether there is a use case for this at all. It may be a hypothetical problem that never occurs in real world, except some artificial tests.

let's not use "SAFE" as name. match whatever new name was similarly chosen in the Python code.

suggested new wording:

:mod:`pickle` now avoids a potential allocated virtual memory
address space denial of service condition when
unpickling specially crafted data.

Pickle isn't for use on untrusted data and no privileges could be escalated, so calling this a vulnerability is a stretch. It also wasn't a bad one, it didn't consume memory, it just mapped virtual address space.

I consumes memory. The mapped virtual address space is filled with data (at least in debug build).

Pickle isn't safe for untrusted data by default, but it can be made safe (by strictly restricting what data it can handle) after resolving this issue.

Note that there is _safe_read in http.client, although ironically it is vulnerable to this kind of issue.

I afraid that name like chunked_file_read could be misleading, as the data is not read by fixed size chunks. I guess this is what leads you to misunderstanding about the O(N^2) behavior.

I consumes memory. The mapped virtual address space is filled with data (at least in debug build).

Pickle isn't safe for untrusted data by default, but it can be made safe (by strictly restricting what data it can handle) after resolving this issue.

It is just an arbitrary constant to make the existing tests pass. It should be larger than 2**16, otherwise we will need to make input data for two tests much larger. I do not think that the real code needs more than 0 or 1. I have no good name for this. The Python code does not need this, as it does not have such limitation and is not vulnerable to this issue (but it is less efficient).

This is the part of this PR that I have the greatest doubts. It can make the unpickler to reject an unusual data that is accepted now, although such data never produced by the pickler (it can still be unpickled with the Python implementation).

I can make the C code more similar to the Python code (as a fallback). But this will add a lot of complexity, and I do not know whether there is a use case for this at all. It may be a hypothetical problem that never occurs in real world, except some artificial tests.

Can you add a comment explaining why this and the next test method result in ([], []) being returned no matter what rather than an error when the values are too large? (I suspect readers with a knowledge of the specific pickle protocol may understand, but it isn't obvious otherwise)

size_t being fed into FromSsize_t could be a problem if size_t idx were large enough to overflow that. is that plausible in this code?

Good catch. Actually, the argument of _Unpickler_MemoGet() is always a Py_size_t casted to size_t, but it is better to always treat it as size_t here.

to be pedantic, check for the possibility of overflow first. (practically speaking: I don't think res or s could ever be large enough for that to actually happen) The existing code being replaced already had this "problem" FWIW.

When I wrote code for all these __sizeof__ implementations, I initially added integer overflow checks. But MvL said me to remove them. If we are going to add integer overflow checks, we should add it not only here, but few lines above (for res += self->memo_size * sizeof(PyObject *)) and in dozens of other places. This is a separate large issue.

Good catch. Actually, the argument of _Unpickler_MemoGet() is always a Py_size_t casted to size_t, but it is better to always treat it as size_t here.

When I wrote code for all these __sizeof__ implementations, I initially added integer overflow checks. But MvL said me to remove them. If we are going to add integer overflow checks, we should add it not only here, but few lines above (for res += self->memo_size * sizeof(PyObject *)) and in dozens of other places. This is a separate large issue.