Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 7, 2019
Merged

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

merged 1 commit into from
Jun 7, 2019

Conversation

jdemeyer
Copy link
Contributor

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

@mangrisano
Copy link
Contributor

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

@mangrisano
Copy link
Contributor

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

@gpshead
Copy link
Member

gpshead commented Jun 6, 2019

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
Copy link
Member

@gpshead gpshead left a comment

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
Copy link
Contributor Author

jdemeyer commented Jun 7, 2019

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
Copy link
Contributor Author

jdemeyer commented Jun 7, 2019

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

@vstinner
Copy link
Member

vstinner commented Jun 7, 2019

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
Objects/classobject.c
@@ -71,7 +71,11 @@ method_vectorcall(PyObject *method, PyObject *const *args,
}
/* use borrowed references */
newargs[0] = self;
memcpy(newargs + 1, args, totalargs * sizeof(PyObject *));
if (totalargs) { /* bpo-37138: if totalargs == 0, then args may be
Copy link
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?

Copy link
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.

Copy link
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
Objects/classobject.c
@@ -71,7 +71,11 @@ method_vectorcall(PyObject *method, PyObject *const *args,
}
/* use borrowed references */
newargs[0] = self;
memcpy(newargs + 1, args, totalargs * sizeof(PyObject *));
if (totalargs) { /* bpo-37138: if totalargs == 0, then args may be
Copy link
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
Copy link
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
Copy link

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