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

Fix bitsize of cmsgHdrLen and msgCtrlLen on Posix #535

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

zeldin
Copy link
Contributor

@zeldin zeldin commented Aug 22, 2022

These fields are size_t, so using CInt on LP64 only accesses 32 bits
of the total 64. This is especially noticable on big endian, where
the lower 32 bits of the number are written to the upper 32 bits of
the field.

@kazu-yamamoto kazu-yamamoto self-requested a review August 24, 2022 01:31
@kazu-yamamoto
Copy link
Collaborator

Good catch.

I think that int comes from macOS which is my main machine. Its manual says that the type is int for this field. And when I apply your patch to master, the test gets stacked.

We need to find a good way both for Linux and macOS.

@kazu-yamamoto
Copy link
Collaborator

kazu-yamamoto commented Aug 24, 2022

Never mind. I was looking at master. (Old text: We should modify cbits/cmsg.c, too.)

@kazu-yamamoto
Copy link
Collaborator

I made a mistake. On macOS, CMSG_LEN is defined:

#define CMSG_LEN(l)             (__DARWIN_ALIGN32(sizeof(struct cmsghdr)) + (l))

So, its return type is size_t.

I'm puzzled why tests fail with this PR.

@zeldin
Copy link
Contributor Author

zeldin commented Aug 24, 2022

Looking at <sys/socket.h> on Monterey, the sizes of the fields are indeed different. cmsg_len and msg_controllen are socklen_t, which is 32 bits, while msg_iovlen is int (meaning that it was mismatched already before)...
Now I kind of wonder what POSIX.1-2008 actually say about these fields. 😟

@kazu-yamamoto
Copy link
Collaborator

The network stack of macOS is a bit strange. Could you put #ifdef to take macOS as a special case?

@zeldin
Copy link
Contributor Author

zeldin commented Aug 24, 2022

Should be possible I guess. Although it's plausible that it's actually Linux that is the special case. macOS is ostensibly based on BSD, which is the source of the sendmsg API. Anyway, I think I need to set up ghc on my mac so that I can run the tests there as well.

@zeldin
Copy link
Contributor Author

zeldin commented Aug 24, 2022

Yes, POSIX.1-2008 supports the macOS definition of the structs, so I guess Linux unilaterally changed the API to fix security issues. Thus, Linux is the special case.

@zeldin
Copy link
Contributor Author

zeldin commented Aug 24, 2022

Actually, rather than hardcoding special cases, I think the best solution would be to just use #type. I'll see if I can cook up something that works.

These fields are size_t on Linux, so using CInt on LP64 only
accesses 32 bits of the total 64.  This is especially noticable on big
endian, where the lower 32 bits of the number are written to the upper
32 bits of the field.  Also use #type socklen_t instead of CUInt/CInt
where applicable, since POSIX does not specify the size of this type.
@zeldin
Copy link
Contributor Author

zeldin commented Aug 24, 2022

There, now the tests pass both on macOS (arm64) and Linux (ppc64) for me. The second force-push was just updating the commit message to be more accurate.
Unfortunately it was not possible to use #type to match the type of a field member (presumable because this would require use of typeof(), which is C2x), so I still had to use #ifdef where Linux used a different type name from what POSIX specifies.

Copy link
Collaborator

@kazu-yamamoto kazu-yamamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks admirable to me!

@kazu-yamamoto kazu-yamamoto merged commit 4d82bf1 into haskell:master Aug 25, 2022
@kazu-yamamoto
Copy link
Collaborator

Merged. Thanks you for your awesome contribution!

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.

2 participants