Skip to content

bpo-37138: fix undefined behaviour with memcpy() on NULL array#13867

Merged
gpshead merged 1 commit into
python:masterfrom
jdemeyer:bpo37138
Jun 7, 2019
Merged

bpo-37138: fix undefined behaviour with memcpy() on NULL array#13867
gpshead merged 1 commit into
python:masterfrom
jdemeyer:bpo37138

Conversation

@jdemeyer

@jdemeyer jdemeyer commented Jun 6, 2019 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@mangrisano

mangrisano commented Jun 6, 2019

Copy link
Copy Markdown
Contributor

@vstinner Would you be able to review this fix, please? Thanks a lot. :)

@mangrisano

mangrisano commented Jun 6, 2019

Copy link
Copy Markdown
Contributor

In any case, I think should be added a test.

@gpshead

gpshead commented Jun 6, 2019

Copy link
Copy Markdown
Member

I'm not sure an explicit test for this is really needed. (nice to have though)

From the bug, this code path was called all the time with the relevant 0 / null based on the undefined behavior sanitizer buildbot output (though I never bothered trying to trace what codepath's triggered it). So this case is covered by something. I'm just not sure explicitly how.

If you feel you can add a test that triggers this specific code path, go for it.

also, i've added the skip news label as this isn't really worth mentioning in news. its an internal only bugfix for a behavior in beta1 that shouldn't be tripping anyone up in reality.

@gpshead gpshead added skip news type-bug An unexpected behavior, bug, or error labels Jun 6, 2019
gpshead
gpshead approved these changes Jun 6, 2019
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving, the code change makes sense. I'm not clicking merge for now in case @jdemeyer wants to add an explicit test. I'll leave that up to Jeroen.

@jdemeyer

jdemeyer commented Jun 7, 2019

Copy link
Copy Markdown
Contributor Author

It occurs for example every time that _PyObject_CallNoArg() is used on a method object. There are plenty of code paths where this happens. To name just one: the implementation of with obj: calls obj.__enter__() (which is typically a method) this way.

@jdemeyer

jdemeyer commented Jun 7, 2019

Copy link
Copy Markdown
Contributor Author

In fact, there is already an explicit test in test_fastcall in Lib/test/test_call.py

@vstinner

vstinner commented Jun 7, 2019

Copy link
Copy Markdown
Member

In fact, there is already an explicit test in test_fastcall in Lib/test/test_call.py

Yep:

                if not args:
                    # args=NULL, nargs=0
                    result = _testcapi.pyobject_fastcall(func, None)
                    self.check_result(result, expected)

I wrote it because this code path caused troubles in the past ;-)

vstinner
vstinner reviewed Jun 7, 2019
View reviewed changes
Comment thread Objects/classobject.c
/* use borrowed references */
newargs[0] = self;
memcpy(newargs + 1, args, totalargs * sizeof(PyObject *));
if (totalargs) { /* bpo-37138: if totalargs == 0, then args may be

@vstinner vstinner Jun 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think that the comment is required.

@gpshead: what do you think?

@jdemeyer jdemeyer Jun 7, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wrote the comment because the reason for that if (totalargs) is totally not obvious. It's an obscure point in the C standard that I was unaware of. I think that there are plenty of developers that do not know that memcpy(dst, NULL, 0) is undefined behaviour.

@gpshead gpshead Jun 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm in favor of such comments. :)

gpshead
gpshead approved these changes Jun 7, 2019
View reviewed changes
Comment thread Objects/classobject.c
/* use borrowed references */
newargs[0] = self;
memcpy(newargs + 1, args, totalargs * sizeof(PyObject *));
if (totalargs) { /* bpo-37138: if totalargs == 0, then args may be

@gpshead gpshead Jun 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm in favor of such comments. :)

@gpshead gpshead merged commit 1f95317 into python:master Jun 7, 2019
@miss-islington

miss-islington commented Jun 7, 2019

Copy link
Copy Markdown
Contributor

Thanks @jdemeyer for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 7, 2019
@jdemeyer @miss-islington
bpo-37138: fix undefined behaviour with memcpy() on NULL array (pytho…
67dcb7e 
…nGH-13867)

(cherry picked from commit 1f95317)

Co-authored-by: Jeroen Demeyer <J.Demeyer@UGent.be>
@bedevere-bot

bedevere-bot commented Jun 7, 2019

Copy link
Copy Markdown

GH-13900 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Jun 7, 2019
@miss-islington @jdemeyer
bpo-37138: fix undefined behaviour with memcpy() on NULL array (GH-13867
6e05307 
)

(cherry picked from commit 1f95317)

Co-authored-by: Jeroen Demeyer <J.Demeyer@UGent.be>
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@jdemeyer @DinoV
bpo-37138: fix undefined behaviour with memcpy() on NULL array (pytho…
0259383 
…nGH-13867)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@gpshead gpshead gpshead approved these changes

Assignees

No one assigned

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jdemeyer @mangrisano @gpshead @vstinner @miss-islington @bedevere-bot @the-knights-who-say-ni