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

Fix CMSG_LEN #54

Merged
merged 3 commits into from Mar 23, 2016
Merged

Fix CMSG_LEN #54

merged 3 commits into from Mar 23, 2016

Conversation

@antrik
Copy link
Contributor

antrik commented Mar 22, 2016

This fixes a bug in the CMSG_LEN function; drops the ugly workaround that was necessitated by it; and adds a series of test cases to help identify possible problems in this (and related) areas.

antrik added 3 commits Mar 11, 2016
A series of tests with messages carrying various numbers of FDs.

This should help finding possible errors in the various calculations
regarding message size etc.

The tests are using large transfers, as these invoke additional size
calculations that need to be checked too.
There was a bracing error in the transcription of the original C macro,
making it effectively equivalent to CMSG_SPACE().

(It would produce an even more weird result on systems where
CMSG_ALIGN(cmsghdr) is not a no-op -- but that seems a rather
hypothetical case, considering the definition of the structure, which
naturally aligns to sizeof size_t both on systems with sizeof size_t ==
sizeof int and with sizeof size_t == 2 * sizeof int...)

This bug resulted in undesired padding on systems where CMSG_ALIGN() is
not a total no-op. (I.e. x86_64, but not i386.)
Do not pad the auxillary data message with /dev/null FDs anymore.

I'm pretty sure this was only ever necessary because of the CMSG_LEN()
bug.

(It resulted in the unoccupied padding space in the auxillary data
buffer to be actually declared as used... Now that this is no longer the
case, there is no need anymore to fill it up with dummy values.)

Tests with fixed and unfixed CMSG_LEN() do seem to confirm this.
@pcwalton
Copy link
Collaborator

pcwalton commented Mar 23, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Mar 23, 2016

📌 Commit 388eb12 has been approved by pcwalton

@bors-servo
Copy link
Contributor

bors-servo commented Mar 23, 2016

Testing commit 388eb12 with merge e3aef45...

bors-servo added a commit that referenced this pull request Mar 23, 2016
Fix CMSG_LEN

This fixes a bug in the `CMSG_LEN` function; drops the ugly workaround that was necessitated by it; and adds a series of test cases to help identify possible problems in this (and related) areas.
@bors-servo
Copy link
Contributor

bors-servo commented Mar 23, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit 388eb12 into servo:master Mar 23, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.