Bug 644003 - cmsg(3) manpage incomplete, broken examples
cmsg(3) manpage incomplete, broken examples
Status: CLOSED INSUFFICIENT_DATA
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: man-pages (Show other bugs)
5.5
All Linux
low Severity medium
: rc
: ---
Assigned To: Peter Schiffer
BaseOS QE - Apps
: ManPageChange, Patch
Depends On:
Blocks: 699650
  Show dependency treegraph
 
Reported: 2010-10-18 14:28 EDT by Hallvard B Furuseth
Modified: 2012-08-29 09:57 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 699650 (view as bug list)
Environment:
Last Closed: 2012-08-29 09:57:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Test program for SCM_RIGHTS (2.52 KB, text/x-csrc)
2010-10-18 14:28 EDT, Hallvard B Furuseth
no flags Details
first draft of cmsg(3) man page (6.11 KB, text/plain)
2012-05-28 11:10 EDT, Peter Schiffer
no flags Details
cmsg.patch (957 bytes, patch)
2012-05-28 11:11 EDT, Peter Schiffer
no flags Details | Diff

  None (edit)
Description Hallvard B Furuseth 2010-10-18 14:28:16 EDT
Created attachment 454185 [details]
Test program for SCM_RIGHTS

Description of problem:

man 3 cmsg has several problems.  Possibly some should be referred
to whoever defined the original CMSG_*(), if anyone does.


The examples use (int *)CMSG_DATA(cmsg).  That can give an
unaligned int*: cmsg points into a char buffer - at least in the
SCM_RIGHTS example, and presumably in both.  They should instead
pass CMSG_DATA(cmsg) to memcpy().


The SCM_RIGHTS example ends with msg.msg_controllen =
cmsg->cmsg_len; which should be ... = CMSG_SPACE(sizeof myfds);
The descriptive text a few paragraphs above gets it right.

Except this statement can presumably be dropped in this case,
since it is the same value as we set originally.

Except in my testing it works to use CMSG_LEN when there's just
one cmsg, and in recvmsg this works to limit the number of file
descriptors received.  The latter is useful, see below.  But since
there's no clear documentation of msg_controllen requrements, I
guess that counts as an unsupported or undocumented hack.


As far as I can tell, neither manpage (cmsg, sendmsg, recvmsg)
mention that at least one msg_iov byte must be sent and received.
Except it might be deduced from recvmsg(2) saying return value =
#bytes received = 0 means socket shutdown.
The SCM_RIGHTS example in particular looks complete, just missing
the sendmsg(), but the recvmsg() call will hang without the sender
iov or fail without the receiver iov.


The cmsg(3) and/or recvmsg(2) manpages should warn that the sender
can inject a file descriptor leak into a sloppy receiver program:
If that accepts msg_control but does not parse all control messages,
checking for and close()ing any unexpected SCM_RIGHTS descriptors.

However, I don't see an "official" way of doing that:

First, there's no documented way to know how many descriptors were
received.  The doc doesn't even say it is possible:  The CMSG_LEN
doc mentions alignment, so it could round up just like CMSG_SPACE.
Anyway, it doesn't on Linux, so I've settled for this:

   enum { cm_offset = CMSG_LEN(sizeof(int)) - sizeof(int) };
   int num_fd = (int)(cmsg->cmsg_len - cm_offset) / (int)sizeof(int);

CMSG_LEN(nonzero) in case it meets some OS where it's not written
to handle zero.  (Assuming several OSes support CMSG_LEN & co and
do it slightly differently, I don't know.)  The (int) casts are so
if the above formula breaks and cmsg_len < cm_offset, I get near
zero and not a huge size_t wraparound.  One could also fill buf[]
with (int)-1 values first, and check that the resulting num_fd > 0.

Second, what should the "is this SCM_RIGHTS?" test be?  As general
as possible in case the sender manages to set some strange values.
I guess it's best written (cmsg->cmsg_type == SCM_RIGHTS) without
the additional (cmsg->cmsg_level==... &&) from the IP_TTL example.
Is that latter test needed?  And is it needed for SCM_RIGHTS?


Version-Release number of selected component (if applicable):
Linux 2.6.18-194.11.4.el5 x86_64

How reproducible:
Always

Steps to Reproduce:
1. Download passfd.c.
2. cc passfd.c -DEXTRA=0 && ./a.out
3. ^C when it hangs, repeat for EXTRA=1, 3, 5, 7, and maybe +8 too.
  
Actual results:
EXTRA=0: sendmsg: Invalid argument. (copied sender controllen from example)
1, 5: Hanging.  (did not send a msg_iov byte.)
3: recvmsg: Operation not supported.
7:   Success, 6 descriptors (5 + 1 from recvmsg CMSG_SPACE alignment)
7+8: Success, 5 descriptors (5     from recvmsg CMSG_LEN in msg_controllen)
any+16 (tried MSG_TRUNC just in case): No difference.

Expected results:

Who knows... Expected some more successes.
Additional info:
Comment 1 Hallvard B Furuseth 2010-10-19 13:51:33 EDT
I wrote:
> The examples use (int *)CMSG_DATA(cmsg).  That can give an
> unaligned int*: cmsg points into a char buffer (...)

Never mind that.  "cmsg->cmsg_len = ..." & co would fail for the
same reason, so CMSG_FIRSTHDR() will have to take care of that by
adjusting the address on hosts that require aligned access.

OTOH the code does get gcc complaints about type aliasing, since
it accesses data via another type than it was declared/created
with. (Accesses char as int, it's the opposite which is allowed.)

I'm hoping someone will someday create a safer interface, but
that's a rather bigger issue than manpage fixes.
Comment 3 RHEL Product and Program Management 2011-06-20 18:11:42 EDT
This request was evaluated by Red Hat Product Management for inclusion in Red Hat Enterprise Linux 5.7 and Red Hat does not plan to fix this issue the currently developed update.

Contact your manager or support representative in case you need to escalate this bug.
Comment 5 RHEL Product and Program Management 2011-09-22 20:30:55 EDT
This request was evaluated by Red Hat Product Management for
inclusion in the current release of Red Hat Enterprise Linux.
Because the affected component is not scheduled to be updated in the
current release, Red Hat is unfortunately unable to address this
request at this time. Red Hat invites you to ask your support
representative to propose this request, if appropriate and relevant,
in the next release of Red Hat Enterprise Linux.
Comment 6 Peter Schiffer 2011-10-19 07:08:12 EDT
This bug wasn't fixed via M-P-O, so changing to the correct component.
Comment 7 Peter Schiffer 2012-05-28 11:10:59 EDT
Created attachment 587252 [details]
first draft of cmsg(3) man page
Comment 8 Peter Schiffer 2012-05-28 11:11:26 EDT
Created attachment 587253 [details]
cmsg.patch
Comment 9 Peter Schiffer 2012-05-28 11:48:20 EDT
Hello Hallvard,

see below my suggestions and comments:

(In reply to comment #0)
> Created attachment 454185 [details]
> Test program for SCM_RIGHTS
> 
> Description of problem:
> 
> man 3 cmsg has several problems.  Possibly some should be referred
> to whoever defined the original CMSG_*(), if anyone does.
> 
> 
> The examples use (int *)CMSG_DATA(cmsg).  That can give an
> unaligned int*: cmsg points into a char buffer - at least in the
> SCM_RIGHTS example, and presumably in both.  They should instead
> pass CMSG_DATA(cmsg) to memcpy().
> 
I am ignoring this issue according to comment 1.
> 
> The SCM_RIGHTS example ends with msg.msg_controllen =
> cmsg->cmsg_len; which should be ... = CMSG_SPACE(sizeof myfds);
> The descriptive text a few paragraphs above gets it right.
> 
> Except this statement can presumably be dropped in this case,
> since it is the same value as we set originally.
> 
> Except in my testing it works to use CMSG_LEN when there's just
> one cmsg, and in recvmsg this works to limit the number of file
> descriptors received.  The latter is useful, see below.  But since
> there's no clear documentation of msg_controllen requrements, I
> guess that counts as an unsupported or undocumented hack.
> 
To fix this, I suggest to remove the last line of that example (msg.msg_controllen = cmsg->cmsg_len;) as it is not necessary there.
> 
> As far as I can tell, neither manpage (cmsg, sendmsg, recvmsg)
> mention that at least one msg_iov byte must be sent and received.
> Except it might be deduced from recvmsg(2) saying return value =
> #bytes received = 0 means socket shutdown.
> The SCM_RIGHTS example in particular looks complete, just missing
> the sendmsg(), but the recvmsg() call will hang without the sender
> iov or fail without the receiver iov.
> 
I suggest to add the sendmsg() call to the SCM_RIGHTS example.
> 
> The cmsg(3) and/or recvmsg(2) manpages should warn that the sender
> can inject a file descriptor leak into a sloppy receiver program:
> If that accepts msg_control but does not parse all control messages,
> checking for and close()ing any unexpected SCM_RIGHTS descriptors.
> 
> However, I don't see an "official" way of doing that:
> 
> First, there's no documented way to know how many descriptors were
> received.  The doc doesn't even say it is possible:  The CMSG_LEN
> doc mentions alignment, so it could round up just like CMSG_SPACE.
> Anyway, it doesn't on Linux, so I've settled for this:
> 
>    enum { cm_offset = CMSG_LEN(sizeof(int)) - sizeof(int) };
>    int num_fd = (int)(cmsg->cmsg_len - cm_offset) / (int)sizeof(int);
> 
> CMSG_LEN(nonzero) in case it meets some OS where it's not written
> to handle zero.  (Assuming several OSes support CMSG_LEN & co and
> do it slightly differently, I don't know.)  The (int) casts are so
> if the above formula breaks and cmsg_len < cm_offset, I get near
> zero and not a huge size_t wraparound.  One could also fill buf[]
> with (int)-1 values first, and check that the resulting num_fd > 0.
> 
To fix this, I would add the following note to the cmsg(3) man page:

The receiver should parse each control message and close every
file descriptor appropriately; otherwise, the sender can inject
a file descriptor leak into the receiver. The number of all
file descriptors passed to the receiver can be computed using the length
of the message, e.g. int nb_of_descriptors = cmsg->cmsg_len / sizeof (int);
in the following examples.
>
> Second, what should the "is this SCM_RIGHTS?" test be?  As general
> as possible in case the sender manages to set some strange values.
> I guess it's best written (cmsg->cmsg_type == SCM_RIGHTS) without
> the additional (cmsg->cmsg_level==... &&) from the IP_TTL example.
> Is that latter test needed?  And is it needed for SCM_RIGHTS?
> 
In my opinion and according to the BSD man page [1], test for SCM_RIGHTS should contain also additional checks for length and level. I don't think that additional checks will break anything, but I really can't tell whether they are required.

I've attached first draft of cmsg(3) man page and the patch showing the difference. Please, do you have any other comments?

Also, I've CC'ed Jeff Law, who helped a lot with these suggestions and Michael Kerrisk, maintainer of man pages who could bring his opinion to these suggestions as well.


[1] http://resin.csoft.net/cgi-bin/man.cgi?section=3&topic=CMSG_DATA
Comment 10 RHEL Product and Program Management 2012-06-11 21:01:41 EDT
This request was evaluated by Red Hat Product Management for
inclusion in the current release of Red Hat Enterprise Linux.
Because the affected component is not scheduled to be updated in the
current release, Red Hat is unfortunately unable to address this
request at this time. Red Hat invites you to ask your support
representative to propose this request, if appropriate and relevant,
in the next release of Red Hat Enterprise Linux.
Comment 11 Peter Schiffer 2012-07-18 10:43:40 EDT
Hallvard, are you still interested in this bug report?
Comment 12 Peter Schiffer 2012-07-30 06:22:43 EDT
ping
Comment 13 Peter Schiffer 2012-08-29 09:57:57 EDT
Feel free to reopen this ticket if you are still interested in fixing it.

Note You need to log in before you can comment on or make changes to this bug.