-
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1) Are the errors that this is checking no longer relevant?
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1) Can you expand this comment to note that we are only dealing with connected sockets due to earlier checks? This threw me off for a while.
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1) If we have a call to connect during this time, how do we guarantee that the contents here won't change? How do we make sure we don't grab data that's changing? It seems like we don't always hold the so_lock during all of these operations.
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1) It looks like it's only true for TCP and UDP, not for all IPv4 and IPv6.
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1) This comment doesn't help. Can you elaborate somewhere on the semantics of this flag?
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1) If anything has set this flag, shouldn't that be an error? Why do we just mask it out and ignore garbage (likely from userland)?
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1) It would help if you explain in the comment why we don't need to do the translation at this point.
-
-
usr/src/uts/common/sys/socket.h (Diff revision 1) It may be worth adding here that callers can't set this, at least as described here. It doesn't seem correct to say that this is for kernel sockets only. As kernel sockets don't set this, rather it's being used internally by the framework.
7590 sendmsg on AF_UNIX socket fails after process drops privileges
Review Request #269 — Created Nov. 18, 2016 and submitted
Information | |
---|---|
gwr | |
illumos-gate | |
7590 | |
Reviewers | |
general | |
7590 sendmsg on AF_UNIX socket fails after process drops privileges
We should not assume we can call so_ux_addr_xlate() in sendmsg,
because we may no longer have the privileges needed to lookup
the AF_UNIX name again. The problem here was with "connected"
datagram sockets, where we already know the peer address in
the internal form needed for sending. Just use it.
The test program attached to the issue.
Added some tests for this to $SRC/tests.
Ran this on a system using AF_UNIX stuff (X11)This build clean, and tests work fine.
I'd like to RTI it. Can I get some "ship it"s?
Change Summary:
Updates for Robert's review. Use separate storage for temporary internal address used by sendmsg, so it doesn't overwrite the (internal form) peer address.
Diff: |
Revision 2 (+177 -62) |
---|
Change Summary:
Fix some minor things I noticed looking over the reivew.
Diff: |
Revision 3 (+180 -63) |
---|
-
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 3) name is defined as struct sockaddr *, so it's just confusing to assign internal form address to name and pass it down to sosend_dgram() and sosend_dgramcmsg(). How about just setting MSG_SENDTO_NOXLATE flag here? name is still assigned sti->sti_faddr_sa, and then inside sosend_dgram() and sosend_dgramcmsg(), if flag MSG_SENDTO_NOXLATE is set, get internal form address from sti->sti_ux_faddr?
I've tested this change using Samba 4.4.7 and it worked well. By the way, I was the bug reporter for the following issue:
https://bugzilla.samba.org/show_bug.cgi?id=12429
-
-
usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 3) Youzhong Yang suggested an alternative here:
addr = &sti->sti_ux_faddr; addrlen = sizeof (sti->sti_ux_faddr);
That would be equivalent, but it would mean that the MSG_SENDTO_NOXLATE would have the additional semantics of ignoring the passed name. I don't think I like that better than passing in name set to the same value, which follows that pattern established with the sti_faddr_noxlate flag.
Change Summary:
Add some test programs (the original "drop_priv" from Jeremy, plus some variations on that).
-
-
usr/src/test/os-tests/runfiles/default.run (Diff revision 5) Why is this commented out? Should we have all of these in gate or not? Or is there something special about them that causes it not to be able to operate?
-
usr/src/test/os-tests/tests/sockfs/Makefile (Diff revision 5) Something tells me this line isn't right. ;)
-
usr/src/test/os-tests/tests/sockfs/conn.c (Diff revision 5) Would we expect listen to succeed multiple times? I'm not sure I understand why this is a while loop.
-
usr/src/test/os-tests/tests/sockfs/drop_priv.c (Diff revision 5) Missing new line between return value and type? This is true of multiple functions here.
Change Summary:
Updates from RM's review.
Diff: |
Revision 6 (+1170 -64)
|
---|
Change Summary:
packaging
Diff: |
Revision 7 (+1174 -64)
|
---|
Change Summary:
fix lint
Diff: |
Revision 8 (+1176 -66)
|
---|
-
-
-
usr/src/test/os-tests/tests/sockfs/drop_priv.c (Diff revision 8) Make this a proper comment and don't silence cstyle? :-) also s/Illumos/illumos/.
-
usr/src/test/os-tests/tests/sockfs/drop_priv.c (Diff revision 8) What is this about? If we aren't going to use these fields, just remove them; if they are showing the problem, remove the
#if 0
, so that problem is visible (even if it makes the test case fail). -
-
-
-
Change Summary:
Yuri's review
Diff: |
Revision 9 (+1169 -66)
|
---|
Change Summary:
Clean up the comment at the top of tests/sockfs/drop_priv.c
Diff: |
Revision 10 (+1147 -66)
|
---|