Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cPickleCache uses sizeof(X) when it means isinstance(X,persistent) #69

Closed
jamadden opened this issue Jan 26, 2018 · 1 comment · Fixed by #70
Closed

cPickleCache uses sizeof(X) when it means isinstance(X,persistent) #69

jamadden opened this issue Jan 26, 2018 · 1 comment · Fixed by #70

Comments

@jamadden
Copy link
Member

From cPickleCache.c's cc_add function:

    else if (v->ob_type->tp_basicsize < sizeof(cPersistentObject))
    {
        /* If it's not an instance of a persistent class, (ie Python
            classes that derive from persistent.Persistent, BTrees,
            etc), report an error.
            TODO:  checking sizeof() seems a poor test.
        */
        PyErr_SetString(PyExc_TypeError,
                        "Cache values must be persistent objects.");
        return -1;
    }

I think I agree with the code's own comment that 'checking sizeof() seems a poor test'. Why does it do that instead of using PyObject_IsInstance (which is what the Python implementation of PickleCache does)? Could that check be changed?

(Ref zopefoundation/Persistence#9)

@jamadden jamadden changed the title cPickleCache uses sizeof() when it means isinstance(,persistent) cPickleCache uses sizeof(X) when it means isinstance(X,persistent) Jan 26, 2018
@tseaver
Copy link
Member

tseaver commented Jan 26, 2018

That change seems "safe" (won't cause coredumps), because the PyObject_IsInstance test should be a strict subset of the size check. I don't know if there is any code in the wild which is using not-actually-instances-of-Persistent objects which are accidentally big enough (and have fakey slots to boot), but ISTM that such hypothetical code Deserves to Lose(TM).

jamadden added a commit that referenced this issue Jul 30, 2018
Instead of testing object sizes.

This matches what the pure Python implementation does and is a
stronger test that the object really is compatible with the cache.
Previously, an object could potentially include ``cPersistent_HEAD``
and *not* set ``tp_base`` to ``cPersistenceCAPI->pertype`` and still
be eligible for the pickle cache; that is no longer the case.

This resolves several compiler warnings:

persistent/cPickleCache.c:521:43: warning: comparison of integers of different signs: 'Py_ssize_t' (aka 'long') and 'unsigned long' [-Wsign-compare]
                (v->ob_type->tp_basicsize >= sizeof(cPersistentObject))
                 ~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~
persistent/cPickleCache.c:709:39: warning: comparison of integers of different signs: 'Py_ssize_t' (aka 'long') and 'unsigned long' [-Wsign-compare]
    else if (v->ob_type->tp_basicsize < sizeof(cPersistentObject))

Fixes #69.
clrpackages referenced this issue in clearlinux-pkgs/persistent Jul 31, 2018
… #66

Charles Merriam (1):
      Update documentation links.

Jason Madden (7):
      Back to development: 4.2.5
      Merge pull request #67 from zopefoundation/issue66
      Change cPickleCache to use PER_TypeCheck
      Merge pull request #70 from zopefoundation/issue69
      Update url in setup.py
      Merge pull request #72 from zopefoundation/issue71
      Preparing release 4.3.0

Jim Fulton (1):
      Merge pull request #68 from merriam/merriam-patch-1

KIMURA Chikahiro (1):
      Fix possibility of rare crash during dealloc. Fixes #66

Marius Gedminas (1):
      Use SVG icon for Travis

Michael Howitz (1):
      No longer use shut down API.

4.3.0 (2018-07-30)
------------------

- Fix the possibility of a rare crash in the C extension when
  deallocating items. See zopefoundation/persistent#66

- Change cPickleCache's comparison of object sizes to determine
  whether an object can go in the cache to use ``PyObject_TypeCheck()``.
  This matches what the pure Python implementation does and is a
  stronger test that the object really is compatible with the cache.
  Previously, an object could potentially include ``cPersistent_HEAD``
  and *not* set ``tp_base`` to ``cPersistenceCAPI->pertype`` and still
  be eligible for the pickle cache; that is no longer the case. See
  `issue 69 <https://github.com/zopefoundation/persistent/issues/69>`_.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants