Opened 4 years ago

Closed 4 years ago

#8912 defect closed fixed (fixed)

The code for receiving UNIX socket doesn't check the ancillary data is of the appropriate type.

Reported by: Tom Prince Owned by: Glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: ex.vitorino@… Branch:
Author:

Description

There are multiple types of ancillary data that can be received on a unix socket, and multiple messages can be received at once. Both of these things should be handled in some manner.

I noticed this when looking over #8911.

Change History (18)

comment:2 Changed 4 years ago by exvito

Cc: ex.vitorino@… added
Keywords: review added

Github PR at https://github.com/twisted/twisted/pull/647

Notes:

  • Inspired by Tom's sketch, logs a message when unsupported ancillary data is received.
  • Other than verifying level and type, the code also ensures all messages are processed, not just the first.

Thanks in advance for input and review.

PS: I feel this should be considered a "defect" type ticket, why did you classify it as "enhancement", Tom?

comment:3 Changed 4 years ago by exvito

Keywords: review removed

Depends on #8969 being fixed so that full tests can be written.

comment:4 Changed 4 years ago by exvito

Keywords: review added
Type: enhancementdefect

That was fast. :) PR up for review at https://github.com/twisted/twisted/pull/652.

Thanks in advance.

PS: Changing type to defect.

comment:5 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to exvito

Thanks again for your contribution, exvito.

PR reviewed at https://github.com/twisted/twisted/pull/652#pullrequestreview-16682879 - please address the feedback and resubmit.

comment:6 Changed 4 years ago by exvito

Keywords: review added

Glyph, thanks for your review.

The latest commits address the issues you pointed out, including factoring the test skipping decision to "outside" the test code. Coverage should be 100% once the Mac buildbots run through them.

Feedback for further improvement is welcome, naturally.

comment:7 Changed 4 years ago by Jean-Paul Calderone

Owner: changed from exvito to Jean-Paul Calderone
Status: newassigned

comment:8 Changed 4 years ago by Jean-Paul Calderone

PS: I feel this should be considered a "defect" type ticket, why did you classify it as "enhancement", Tom?

In practice, the difference doesn't really affect anything. Given that it's academic, I would go along with the idea that this is an enhancement: the code previously functioned exactly as intended when used as intended - that is, to exchange file descriptors. The enhancement is that now, if you send something _other_ than a file descriptor, it won't trip the code up. So the functionality has been increased but the prior functionality continues to operate exactly as it was. Enhancement.

But as I said, it doesn't really matter.

comment:9 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to exvito
Status: assignednew

comment:10 in reply to:  8 Changed 4 years ago by exvito

Keywords: review added

Replying to Jean-Paul Calderone:

In practice, the difference doesn't really affect anything. Given that it's academic, I would go along with the idea that this is an enhancement: the code previously functioned exactly as intended when used as intended - that is, to exchange file descriptors. The enhancement is that now, if you send something _other_ than a file descriptor, it won't trip the code up. So the functionality has been increased but the prior functionality continues to operate exactly as it was. Enhancement.

But as I said, it doesn't really matter.

Agreed, the bottom line is it really doesn't matter.

I've updated the code and commented / updated the PR per each of your comments. Thanks again.

comment:11 Changed 4 years ago by exvito

Owner: exvito deleted

comment:12 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to exvito

Thanks for pressing forward, exvito.

Good news:

The new test really does test the behavior is correct.

Bad news:

Thanks to its testing, the OS X builder does actually fail because the expected values don't match.

Please address the failing test on github and submit for re-review. If repo:write permission would be helpful in doing this, just let me know.

comment:13 Changed 4 years ago by exvito

Glyph,

Thanks for the review and feedback.

I went through the failure log which surprised me, somehow. After about one hour of diagnostic I suspect we're hitting a kernel regression in Mac OS X 10.10 vs. 10.9 (not sure about later ones as I have none readily available to try out).

Facts about the failing test:

  • It verifies the order of the sent/received FDs is respected based on fstat's st_dev/st_ino.
  • On 10.9 it always succeeds.
  • On 10.10 it fails the first time after reboot and succeeds on subsequent runs.
  • Interesting: Both Mac OS kernels seem to assign -1 to st_dev and uncommon low integers to st_ino.
  • After reboot, running the test:
    • On 10.9, the FDs obtained via recvmsg have inodes 1 and 2, matching the inodes of the sent FDs in the same order.
    • On 10.10, the FDs obtained via recvmsg have inodes 0 and 1, not matching the inodes of the sent FDs that have inodes 2 and 1 (yes, different from 10.9).
  • Running the test a second time:
    • Both kernels get FDs with inodes 3 and 4, matching the inodes of the sent FDs in the same order.

Having said this, 10.10 is showing a very strange behavior, not only because of the apparent "off-by-one" inode for the received FDs (0, 1) but also because of the order of the inodes associated to the sent FDs (2, 1). Adding to the mystery, subsequent runs go smoothly and, somewhat unsurprisingly, show matching inode pairs (3, 4), (5, 6), (7, 8) and so on for both the sent and received FDs.

Facing this, I explored the idea of comparing/validating sent/received FDs based on socket.getsockname(). This has resulted in a dead-end: in both Mac OS 10.10 and 10.9 getsockname() returns truncated UNIX socket names like '/var/private/x' instead of the full name '/var/private/xjhsdjhasjhd/asjdhajshd/send.socket'. This prevents any kind of useful comparison.

For now, I'm not sure how to move forward. Alternatives may be i) using FDs associated to files instead of FDs associated to UNIX sockets in the tests or ii) faking recvmsg for all these tests pretty much like it's being done for the "bad/unsupported" CMSG level/type case. I'll give it a little rest and get back to it early next week, if not sooner, trying to figure out a reliable, cross-UNIX approach of confirming two FDs "point" to the same "thing".

Any input is very welcome.

PS: To be technically correct, s/inode/inode number/g. :)

comment:14 Changed 4 years ago by exvito

Keywords: review added
Owner: exvito deleted

Adjusted test to send/receive/verify file FDs instead of socket FDs.

Test code comes out shorter (and maybe cleaner) and, most importantly, succeeds in verifying what it has to verify on Linux, FreeBSD 11 and Mac OS X 10.9 + 10.10. At least per the trials I ran on the platforms I have available.

Let's see what travis + buildbot say.

Thanks for guidance and reviewing.

Last edited 4 years ago by exvito (previous) (diff)

comment:15 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to exvito

Approving review here: https://github.com/twisted/twisted/pull/652#pullrequestreview-19226752

Tests kicked off.

Third time's the charm!

Reassigning to exvito so it's clear that it's their responsibility to poke someone to actually do the merge if I don't :).

comment:16 Changed 4 years ago by exvito

Keywords: review added
Owner: exvito deleted

This has beern reviewed and approved but I'm asking for another quick round of review and poking committers to either accept and merge or feedback on the last two small aspects I commented in the PR:

  • codecov test failing because it is considering a non-changed line.
  • topfile wording.

Thanks.

comment:17 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Glyph

Looks. Good. To. Me.

LANDING.

comment:18 Changed 4 years ago by Glyph <glyph@…>

Resolution: fixed
Status: newclosed

In 3544422c:

Merge pull request #652 from exvito/8912-exvito-safer-unix-do-read-tk2

Author: exvito

Reviewer: glyph

Fixes: ticket:8912

UNIX socket endpoints now process all messages from recvmsg's ancillary data via twisted.internet.unix.Server.doRead/twisted.internet.unix.Client.doRead, while discarding and logging ones that don't contain file descriptors.

Note: See TracTickets for help on using tickets.