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

Avoid narrowing conversion in image_wrapper on 32-bit. #10736

Merged
merged 1 commit into from Mar 9, 2018

Conversation

Projects
None yet
4 participants
@anntzer
Copy link
Contributor

commented Mar 9, 2018

PR Summary

@yurivict can you check whether that fixes #10698?

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way
@yurivict

This comment has been minimized.

Copy link

commented Mar 9, 2018

Still fails:

src/_image_wrapper.cpp:422:24: error: non-constant-expression cannot be narrowed from type 'unsigned int' to 'npy_intp' (aka 'int') in initializer list [-Wc++11-narrowing]
    npy_intp dim[3] = {rows, cols, 4};
                       ^~~~
@anntzer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

same fix applies there... pushed it

@anntzer anntzer force-pushed the anntzer:narrowing branch from c61b0f2 to 7111817 Mar 9, 2018

@yurivict

This comment has been minimized.

Copy link

commented Mar 9, 2018

Now it succeeds, thanks!

@tacaswell

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

Just to check, the issue here is that we are implicitly casting a uint -> int which chops off half of the range of the uint and we were probably using uint because the rows / columns should always be positive. Now we are doing the cast explicitly in the PyArg_ParseTuple.

If this was going to cause any issue with the maximum size image we could render, it already would be?

@tacaswell tacaswell added this to the v2.2.1 milestone Mar 9, 2018

@tacaswell

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

Candidate for backport to 2.2.x because this fixes a 'does not compile' bug.

@anntzer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

No, the issue is that (at the npy_intp dim[3] = {rows, cols, 4} line) we were casting an unsigned int (rows) to a npy_intp (intptr_t), which goes from 64bit to 32bit on the OP's architecture (I know this because the ft2 PR had the same issue when I made it work on circleci...), and that's something a compiler is required by the standard to flag (at least with a warning).

Now rows is declared as a npy_intp so everything has the same size. As for PyArg_ParseTuple, the compiler has no knowledge of that function and will happily accept any pointers passed in, but you'll get a crash at runtime if the pointer sizes don't match the format spec. In fact there is indeed an conversion from Py_ssize_t (signed) to npy_intp (unsigned, but at least in practice sizeof(size_t) == sizeof(intptr_t) on all common architechtures nowadays) going on, but that's not something crashing the compiler.

Now that means that yes, if someone passes a negative value for rows, it will be silently wrapped around to a yuge positive value. But in fact the unsigned convertors in PyArg_ParseTuple don't do any overflow checking (https://bugs.python.org/issue33033) so that issue was already present before; the PR actually slightly improves the situation because at least the signed n convertor does overflow checking and we now error cleanly on wildly out-of-range input.

@jklymak

jklymak approved these changes Mar 9, 2018

@jklymak jklymak merged commit 5b7a21e into matplotlib:master Mar 9, 2018

8 checks passed

ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
ci/circleci: docs-python36 Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing c85b029...7111817
Details
codecov/project/library 68.58% (target 50%)
Details
codecov/project/tests 98.56% remains the same compared to c85b029
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details

meeseeksdev bot pushed a commit that referenced this pull request Mar 9, 2018

@anntzer anntzer deleted the anntzer:narrowing branch Mar 9, 2018

tacaswell added a commit that referenced this pull request Mar 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.