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

Do not access uninitialized memory in getink. #1762

Closed

Conversation

lambdafu
Copy link
Contributor

@lambdafu lambdafu commented Mar 8, 2016

The getink code is a bloodbath, but more importantly, it accesses invalid memory in certain code paths:

File Edit Options Buffers Tools Help                                                                                                                                           
_imaging.c: In function ‘getink’:
_imaging.c:474:9: warning: ‘r’ may be used uninitialized in this function [-Wmaybe-uninitialized]
     int r, g, b, a;
              ^

Valgrind complains:

[Pillow]$ PYTHONPATH=. valgrind python Tests/test_image.py
==23998== Conditional jump or move depends on uninitialised value(s)
==23998==    at 0x11C0FB0D: getink (_imaging.c:495)
==23998==    by 0x11C1054C: _draw_ink (_imaging.c:2375)
==23998==    by 0x4F178BD: PyEval_EvalFrameEx (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4F186B3: PyEval_EvalCodeEx (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4F175C5: PyEval_EvalFrameEx (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4F186B3: PyEval_EvalCodeEx (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4F175C5: PyEval_EvalFrameEx (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4F17665: PyEval_EvalFrameEx (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4F186B3: PyEval_EvalCodeEx (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4EA45AB: ??? (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4E7FB02: PyObject_Call (in /usr/lib64/libpython2.7.so.1.0)
==23998==    by 0x4E8E95B: ??? (in /usr/lib64/libpython2.7.so.1.0)

The patch fixes it and removes some redundancies. I stopped short of rewriting the whole function (but I am not saying it would be a bad idea :).

@wiredfool
Copy link
Member

See also #1663, which is also stuck on failing in appveyor.

@wiredfool wiredfool closed this Mar 8, 2016
@wiredfool wiredfool reopened this Mar 8, 2016
@lambdafu
Copy link
Contributor Author

lambdafu commented Mar 8, 2016

I'm fine with closing this, #1663 covers it well. Sorry, I didn't check too closely the existing reports.

@wiredfool
Copy link
Member

No worries. I'll take a look and see how our patches compare. Unfortunately, they appear to be both failing on windows, which is why the other one is stalled.

@wiredfool
Copy link
Member

I've found the bug on windows in the other PR, and it looks to be more complete, so closing this one in favor of #1663.

@wiredfool wiredfool closed this Mar 10, 2016
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 this pull request may close these issues.

2 participants