Skip to content

bpo-34395: Don't free allocated memory on realloc fail in load_mark() in _pickle.c.#8788

Merged
serhiy-storchaka merged 3 commits intopython:masterfrom
sir-sigurd:bpo-34395
Aug 25, 2018
Merged

bpo-34395: Don't free allocated memory on realloc fail in load_mark() in _pickle.c.#8788
serhiy-storchaka merged 3 commits intopython:masterfrom
sir-sigurd:bpo-34395

Conversation

@sir-sigurd
Copy link
Contributor

@sir-sigurd sir-sigurd commented Aug 16, 2018 •
edited by bedevere-bot
Loading

serhiy-storchaka
serhiy-storchaka reviewed Aug 16, 2018
View reviewed changes
Modules/_pickle.c Outdated
@@ -6297,14 +6297,13 @@ load_mark(UnpicklerObject *self)
return -1;
}
Copy link
Member

@serhiy-storchaka serhiy-storchaka Aug 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the check above can be removed. alloc > (PY_SSIZE_T_MAX / sizeof(Py_ssize_t)) is duplicated in PyMem_RESIZE(), and failing alloc <= ((size_t)self->num_marks + 1) is not possible if sizeof(Py_ssize_t) > 2.

Copy link
Contributor Author

@sir-sigurd sir-sigurd Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpython/Modules/_pickle.c

Line 6289 in 1590c39

if ((self->num_marks + 1) >= self->marks_size) {

Also, it seems that new space is needed only when self->num_marks + 1 is strictly greater than self->marks_size.

Copy link
Member

@serhiy-storchaka serhiy-storchaka Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. This condition can be written as self->num_marks >= self->marks_size or self->num_marks == self->marks_size.

Copy link
Contributor Author

@sir-sigurd sir-sigurd Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serhiy-storchaka
I'd prefer to make these change as separate commits/RPs as they are not related to this bpo issue.

Copy link
Contributor Author

@sir-sigurd sir-sigurd Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR for upsizing issue #8860.

@zhangyangyu
Copy link
Member

zhangyangyu commented Aug 25, 2018

Need backport to make code consistent or as enhancement?

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 25, 2018

I don't think there is a bug that needs to be fixed. This change looks rather as a code clean up to me. If I'm wrong, and it fixes a real bug, it should be backported.

@sir-sigurd, please remove also the redundant check before PyMem_RESIZE if it is truly unneeded.

@sir-sigurd
Copy link
Contributor Author

sir-sigurd commented Aug 25, 2018

@serhiy-storchaka

I'm considering adding this as explanation why overflow check is not needed

assert((size_t)self->num_marks <= PY_SSIZE_T_MAX / sizeof(Py_ssize_t));
Py_BUILD_ASSERT((size_t)PY_SSIZE_T_MAX / sizeof(Py_ssize_t) << 1 <= SIZE_MAX - 20);

but I'm not sure if it makes things clear.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 25, 2018

I think this is not needed. Just remove the check.

@serhiy-storchaka serhiy-storchaka merged commit 90555ec into python:master Aug 25, 2018
@sir-sigurd sir-sigurd deleted the bpo-34395 branch August 25, 2018 10:42
@zhangyangyu
Copy link
Member

zhangyangyu commented Aug 25, 2018 •
edited
Loading

failing alloc <= ((size_t)self->num_marks + 1) is not possible if sizeof(Py_ssize_t) > 2.

@serhiy-storchaka Can this be guaranteed in CPython? Wikipedia says size_t is at least 16 bit wise.

CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request Aug 27, 2018
@CuriousLearner
Merge branch 'master' into fix-issue24255
c50bb33 
* master: (104 commits)
  Fast path for exact floats in math.hypot() and math.dist() (pythonGH-8949)
  Remove AIX workaround test_subprocess (pythonGH-8939)
  bpo-34503: Fix refleak in PyErr_SetObject() (pythonGH-8934)
  closes bpo-34504: Remove the useless NULL check in PySequence_Check(). (pythonGH-8935)
  closes bpo-34501: PyType_FromSpecWithBases: Check spec->name before dereferencing it. (pythonGH-8930)
  closes bpo-34502: Remove a note about utf8_mode from sys.exit() docs. (pythonGH-8928)
  Remove unneeded PyErr_Clear() in _winapi_SetNamedPipeHandleState_impl() (pythonGH-8281)
  Fix markup in stdtypes documentation (pythonGH-8905)
  bpo-34395: Don't free allocated memory on realloc fail in load_mark() in _pickle.c. (pythonGH-8788)
  Fix upsizing of marks stack in pickle module. (pythonGH-8860)
  bpo-34171: Prevent creating Lib/trace.cover when run the trace module. (pythonGH-8841)
  closes bpo-34493: Objects/genobject.c: Add missing NULL check to compute_cr_origin() (pythonGH-8911)
  Fixed typo with asynccontextmanager code example (pythonGH-8845)
  bpo-34426: fix typo (__lltrace__ -> __ltrace__) (pythonGH-8822)
  bpo-13312: Avoid int underflow in time year. (pythonGH-8912)
  bpo-34492: Python/coreconfig.c: Fix _Py_wstrlist_copy() (pythonGH-8910)
  bpo-34448: Improve output of usable wchar_t check (pythonGH-8846)
  closes bpo-34471: _datetime: Add missing NULL check to tzinfo_from_isoformat_results. (pythonGH-8869)
  bpo-6700: Fix inspect.getsourcelines for module level frames/tracebacks (pythonGH-8864)
  Fix typo in the dataclasses's doc (pythonGH-8896)
  ...
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 27, 2018

I think we can assume that size_t is at least 32 bit wise, and char is 8 bit wise. Many things in CPython implementation will be broken if this is not true.

@sir-sigurd
Copy link
Contributor Author

sir-sigurd commented Aug 27, 2018

If I'm not mistaken, it can't overflow even if sizeof(Py_ssize_t) == 2 (presuming that CPython allocates at most PY_SSIZE_T_MAX bytes).

@zhangyangyu
Copy link
Member

zhangyangyu commented Aug 27, 2018

If I'm not mistaken, it can't overflow even if sizeof(Py_ssize_t) == 2 (presuming that CPython allocates at most PY_SSIZE_T_MAX bytes).

I don't get it, how it's not possible.

And rechecking the patch it seems to me the overflow check is not right. You should check overflow before doing math operation. size_t alloc = ((size_t)self->num_marks << 1) + 20; might already overflowed and succeed in the overflow check in PyMem_Resize. What do you think about it? Am I mistaken something?

@sir-sigurd
Copy link
Contributor Author

sir-sigurd commented Aug 27, 2018

@zhangyangyu
Presuming that CPython allocates at most PY_SSIZE_T_MAX bytes, we can assume that (size_t)self->num_marks <= PY_SSIZE_T_MAX / sizeof(Py_ssize_t). So
alloc <= (PY_SSIZE_T_MAX / sizeof(Py_ssize_t) << 1) + 20. If sizeof(Py_ssize_t) == 2, then alloc <= PY_SSIZE_T_MAX + 20 and it's obviously less than SIZE_MAX.

Makes sense?

@zhangyangyu
Copy link
Member

zhangyangyu commented Aug 27, 2018

@sir-sigurd Good insight! Thanks for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@zhangyangyu zhangyangyu zhangyangyu approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sir-sigurd @zhangyangyu @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot