Skip to content

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

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

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

Conversation

@jdemeyer
Copy link
Copy Markdown
Contributor

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

@mangrisano
Copy link
Copy Markdown
Contributor

mangrisano commented Jun 6, 2019

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

@mangrisano
Copy link
Copy Markdown
Contributor

mangrisano commented Jun 6, 2019

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

@gpshead
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
/* 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
Copy Markdown
Member

@vstinner vstinner Jun 7, 2019

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

@jdemeyer jdemeyer Jun 7, 2019

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

@gpshead gpshead Jun 7, 2019

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
/* 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
Copy Markdown
Member

@gpshead gpshead Jun 7, 2019

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
Copy Markdown
Contributor

miss-islington commented Jun 7, 2019

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
Copy Markdown

bedevere-bot commented Jun 7, 2019

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