Make it a complex, but a single for(;;) statement. The previous cycle
with some loop logic in the beginning and some loop logic at the end
was confusing. Both me and markj@ were misleaded to a conclusion that
some checks are unnecessary, while they actually were necessary.
Details
Details
- Reviewers
markj - Group Reviewers
network transport - Commits
- rGd97922c6c632: unix/*: rewrite unp_internalize() cmsg parsing cycle
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 45802 Build 42690: arc lint + arc unit
Event Timeline
Comment Actions
What does "unix/*:" mean in the commit title? Why not just "unix:"?
| sys/kern/uipc_usrreq.c | ||
|---|---|---|
| 2640 | ||
| 2642 | Hmm, I think there is a sneaky bug here, also present in the old version. Note that on LP64 platforms, CMSG_SPACE(0) > sizeof(struct cmsghdr), since message data is aligned to 8 bytes and sizeof(struct cmsghdr) == 12. So it's possible to have data - (char *)cm > cm->cmsg_len. That means that the calculation of datalen can underflow. For example, set msg_controllen = CMSG_SPACE(0) and msg_control = &(struct cmsghdr){.cmsg_len = sizeof(struct cmsghdr), .cmsg_level = SOL_SOCKET, .cmsg_type = SCM_foo}. I think in this case it's mostly harmless because the test CMSG_SPACE(newlen) > MCLBYTES below will fail, and for non SCM_RIGHTS messages, CMSG_SPACE(datalen) will equal CMSG_SPACE(0) due to 32-bit integer truncation, so the loop will break (though we do not signal an error). But we should definitely fix this. | |
Comment Actions
- Handle incorrectly filled cmsg_len
- Add test case for this
- Rebase it independent from my unix-dgram branch
Comment Actions
Thanks! Looks ok to me, my commits are minor.
| sys/kern/uipc_usrreq.c | ||
|---|---|---|
| 2642 | Can't it be part of the for loop condition? | |
| tests/sys/kern/unix_passfd_test.c | ||
| 748 ↗ | (On Diff #106548) | This comment is too specific now, probably the second sentence can just be deleted. |
| 775 ↗ | (On Diff #106548) | CMSG_LEN(0) is exactly equal to sizeof(struct cmsghdr). |
| 783 ↗ | (On Diff #106548) | Rather than having an ifdef LP64 (I believe this is discouraged in new code since at least on CHERI, sizeof(long) != sizeof(void *)), we could use the predicate CMSG_SPACE(0) != CMSG_LEN(0) or CMSG_SPACE(0) != sizeof(struct cmsghdr). |
| tests/sys/kern/unix_passfd_test.c | ||
|---|---|---|
| 775 ↗ | (On Diff #106548) | Nope: (kgdb) p sizeof(struct cmsghdr) $1 = 12 (kgdb) p (((__uintptr_t)(sizeof(struct cmsghdr)) + 7) & ~7) $2 = 16 |
| tests/sys/kern/unix_passfd_test.c | ||
|---|---|---|
| 775 ↗ | (On Diff #106548) | Oops, sorry, I forgot that CMSG_LEN includes alignment for the header. |
| tests/sys/kern/unix_passfd_test.c | ||
|---|---|---|
| 783 ↗ | (On Diff #106548) | Unfortunately we can't use sizeof() in preprocessor. Do you want me to code the check in C? |
| tests/sys/kern/unix_passfd_test.c | ||
|---|---|---|
| 783 ↗ | (On Diff #106548) | Yeah, I think checking in C is fine... |
| tests/sys/kern/unix_passfd_test.c | ||
|---|---|---|
| 783 ↗ | (On Diff #106548) | Then, I think, this is final version. |