Skip to content

bpo-34521: Fix tests in test_socket to use correctly CMSG_LEN #9594

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

Merged
merged 1 commit into from
Sep 27, 2018

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Sep 26, 2018

After some failures in AMD64 FreeBSD CURRENT Debug 3.x buildbots
regarding tests in test_socket that are using
testFDPassSeparateMinSpace(), FreeBDS revision 337423 was pointed
out to be the reason the test started to fail.

A close examination of the manpage for cmsg_space(3) reveals that
the number of file descriptors needs to be taken into account when
using CMSG_LEN().

https://linux.die.net/man/3/cmsg_space

Example from the manpage. The code below passes an array of file descriptors over a UNIX domain socket using SCM_RIGHTS:

struct msghdr msg = {0};
struct cmsghdr *cmsg;
int myfds[NUM_FD]; /* Contains the file descriptors to pass. */
char buf[CMSG_SPACE(sizeof myfds)];  /* ancillary data buffer */
int *fdptr;

msg.msg_control = buf;
msg.msg_controllen = sizeof buf;
cmsg = CMSG_FIRSTHDR(&msg);
cmsg->cmsg_level = SOL_SOCKET;
cmsg->cmsg_type = SCM_RIGHTS;
cmsg->cmsg_len = CMSG_LEN(sizeof(int) * NUM_FD);   <----------
/* Initialize the payload: */
fdptr = (int *) CMSG_DATA(cmsg);
memcpy(fdptr, myfds, NUM_FD * sizeof(int));
/* Sum of the length of all control messages in the buffer: */
msg.msg_controllen = cmsg->cmsg_len;

https://bugs.python.org/issue34521

@pablogsal pablogsal self-assigned this Sep 26, 2018
@pablogsal pablogsal requested a review from vstinner September 26, 2018 21:07
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting merge labels Sep 26, 2018
After some failures in AMD64 FreeBSD CURRENT Debug 3.x buildbots
regarding tests in test_socket that are using
testFDPassSeparateMinSpace(), FreeBDS revision 337423 was pointed
out to be the reason the test started to fail.

A close examination of the manpage for cmsg_space(3) reveals that
the number of file descriptors needs to be taken into account when
using CMSG_LEN().
@pablogsal
Copy link
Member Author

Manually checking on the buildbot itself reveals that all test for test_socket now pass:

CURRENT-amd64% uname -a
FreeBSD CURRENT-amd64 12.0-ALPHA6 FreeBSD 12.0-ALPHA6  r338678  amd64

CURRENT-amd64% ./python -m test test_socket
Run tests sequentially
0:00:00 load avg: 1.69 [1/1] test_socket

== Tests result: SUCCESS ==

1 test OK.

Total duration: 28 sec 611 ms
Tests result: SUCCESS

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@pablogsal pablogsal merged commit 7291108 into python:master Sep 27, 2018
@pablogsal pablogsal deleted the bpo34521 branch September 27, 2018 09:25
@pablogsal
Copy link
Member Author

I will wait for the buildbots before evaluating if we should backport this or not.

@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 27, 2018
After some failures in AMD64 FreeBSD CURRENT Debug 3.x buildbots
regarding tests in test_socket that are using
testFDPassSeparateMinSpace(), FreeBDS revision 337423 was pointed
out to be the reason the test started to fail.

A close examination of the manpage for cmsg_space(3) reveals that
the number of file descriptors needs to be taken into account when
using CMSG_LEN().

This commit fixes tests in test_socket to use correctly CMSG_LEN, taking
into account the number of FDs.
(cherry picked from commit 7291108)

Co-authored-by: Pablo Galindo <[email protected]>
@bedevere-bot
Copy link

GH-9597 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 27, 2018
After some failures in AMD64 FreeBSD CURRENT Debug 3.x buildbots
regarding tests in test_socket that are using
testFDPassSeparateMinSpace(), FreeBDS revision 337423 was pointed
out to be the reason the test started to fail.

A close examination of the manpage for cmsg_space(3) reveals that
the number of file descriptors needs to be taken into account when
using CMSG_LEN().

This commit fixes tests in test_socket to use correctly CMSG_LEN, taking
into account the number of FDs.
(cherry picked from commit 7291108)

Co-authored-by: Pablo Galindo <[email protected]>
@bedevere-bot
Copy link

GH-9598 is a backport of this pull request to the 3.6 branch.

@miss-islington
Copy link
Contributor

Sorry, @pablogsal, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 7291108d88ea31d205da4db19d202d6cbffc6d93 2.7

@pablogsal
Copy link
Member Author

Good job, @miss-islington!

@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington added a commit that referenced this pull request Sep 27, 2018
After some failures in AMD64 FreeBSD CURRENT Debug 3.x buildbots
regarding tests in test_socket that are using
testFDPassSeparateMinSpace(), FreeBDS revision 337423 was pointed
out to be the reason the test started to fail.

A close examination of the manpage for cmsg_space(3) reveals that
the number of file descriptors needs to be taken into account when
using CMSG_LEN().

This commit fixes tests in test_socket to use correctly CMSG_LEN, taking
into account the number of FDs.
(cherry picked from commit 7291108)

Co-authored-by: Pablo Galindo <[email protected]>
miss-islington added a commit that referenced this pull request Sep 27, 2018
After some failures in AMD64 FreeBSD CURRENT Debug 3.x buildbots
regarding tests in test_socket that are using
testFDPassSeparateMinSpace(), FreeBDS revision 337423 was pointed
out to be the reason the test started to fail.

A close examination of the manpage for cmsg_space(3) reveals that
the number of file descriptors needs to be taken into account when
using CMSG_LEN().

This commit fixes tests in test_socket to use correctly CMSG_LEN, taking
into account the number of FDs.
(cherry picked from commit 7291108)

Co-authored-by: Pablo Galindo <[email protected]>
@vstinner
Copy link
Member

vstinner commented Sep 27, 2018

@pablogsal: It seems like the merged commit message lacks "bpo-34521".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants