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

[#8912] Safer, better approach decoding UNIX socket recvmsg ancillary data #647

Closed
wants to merge 9 commits into from

Conversation

exvito
Copy link
Contributor

@exvito exvito commented Dec 26, 2016

Addresses http://twistedmatrix.com/trac/ticket/8912.

Notes:

  • Inspired by Tom Prince's sketch, code logs a message if unsupported ancillary data is received.
  • PR improves processing of ancillary data in two ways:
    • Now validates level and type before decoding file descriptors from the data (which would otherwise be wrong and prone to all sorts of "interesting" failures by producing invalid/unpredictable descriptors). :)
    • Processes all received ancillary data messages (previous code would only process the first).

Looking for feedback on:

  • Ancillary data processing code is getting long vs. the "recvmsg + return data". Should it be factored out to another method, called from doRead? If so, name suggestions?
  • Is there any recommended way of testing that a given log message was logged? (so that a test for ignored incoming ancillary data case can check that)

Thanks in advance for input and guidance.

@codecov-io
Copy link

codecov-io commented Dec 27, 2016

Current coverage is 90.67% (diff: 86.48%)

Merging #647 into trunk will decrease coverage by 0.53%

@@              trunk       #647   diff @@
==========================================
  Files           836        813     -23   
  Lines        146496     145606    -890   
  Methods           0          0           
  Messages          0          0           
  Branches      12988      12859    -129   
==========================================
- Hits         133609     132021   -1588   
- Misses        10652      11311    +659   
- Partials       2235       2274     +39   

Powered by Codecov. Last update 98a3df2...653f2fe

@exvito
Copy link
Contributor Author

exvito commented Dec 27, 2016

Update:

  • Refactored existing multi fileDescriptorReceived test to verify two cases:
    • Single send/recvmsg with one CMSG with two FDs.
    • Single send/recvmsg with two CMSGs with one FD each.
  • These ensure the loop in the code is tested and works as expected.
  • Found that on Mac OS 10.9.5 sendmsg fails when trying to send ancillary data with two CMSGs. This works on Linux, though. Decided to raise SkipTest at test runtime if sendmsg fails - not sure if this is the best approach, but continuing after such failure would be pointless and letting the error through would be misleading because the failure is not related to the code under test; it is preventing it from being tested, instead.
  • Explored the idea of building an unsupported ancillary CMSG (with level!=SOL_SOCKET and/or type!=SCM_RIGHTS) to test the increased robustness of the code. Hit a wall trying to do that: obviously the kernel won't accept any fake level/type on sendmsg input. A valid, but unsupported, message needs to be built to test the remaining code. Ideally, such message would cross-platform and work on Linux, BSDs and Mac OS.

Lastly:

  • I'm having trouble understanding Travis CI's failures on Python 2 and how this PR relates to them. Running trial here with both Python 2.7 and 3.5 shows no failures on these tests.
  • This was a tough one: sendmsg on Python 2.7 on Linux was segfaulting when given more than one CMSG. Also, due to glibc's CMSG_NXTHDR implementation, the underlying control message buffer needed to be zeroed so as to ensure the proper handling of more than one CMSG. Including those to fixes here for the sake of completeness, but maybe they belong somewhere else (where? fact: no existing Twisted code currently calls sendmsg with more than one CMSG, other than one of these tests).

As before, thanks in advance for feedback and guidance, which is very welcome as per the status above.

@exvito
Copy link
Contributor Author

exvito commented Dec 29, 2016

Cancelling PR, for the time being:

Thanks, hoping to be back soon. :)

@exvito exvito closed this Dec 29, 2016
@exvito exvito deleted the 8912-exvito-safer-unix-do-read branch July 12, 2017 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants