Opened 3 years ago

Closed 3 years ago

#12737 closed bug (fixed)

fd passing functionality fails on x86_64

Reported by: korli Owned by: axeld
Priority: normal Milestone: Unscheduled
Component: Network & Internet/Stack Version: R1/Development
Keywords: Cc:
Blocked By: Blocking: #12736
Has a Patch: no Platform: x86-64

Description

Dovecot requires the ability to pass a file descriptor between two processes.

This is done through local network sockets and looks implemented in Haiku and even works on x86 (see here )

gcc -o conftest -std=gnu99 -D_BSD_SOURCE -Wall -W -Wmissing-prototypes -Wmissing-declarations -Wpointer-arith -Wchar-subscripts -Wformat=2 -Wbad-function-cast -fno-builtin-strftime -Wstrict-aliasing=2 fdpass.c  conftest.c -lnetwork

conftest exits with code 2 on x86_64. Attached are conftest.c and fdpass.c as example of checking the functionality.

Attachments (3)

conftest.c (3.7 KB) - added by korli 3 years ago.
fdpass.c (5.8 KB) - added by korli 3 years ago.
fdpass.h (637 bytes) - added by korli 3 years ago.

Download all attachments as: .zip

Change History (10)

Changed 3 years ago by korli

Attachment: conftest.c added

Changed 3 years ago by korli

Attachment: fdpass.c added

Changed 3 years ago by korli

Attachment: fdpass.h added

comment:1 Changed 3 years ago by korli

Following patch fixes the issue:

diff --git a/src/add-ons/kernel/network/stack/net_socket.cpp b/src/add-ons/kernel/network/stack/net_socket.cpp
index 5582351..b447a99 100644
--- a/src/add-ons/kernel/network/stack/net_socket.cpp
+++ b/src/add-ons/kernel/network/stack/net_socket.cpp
@@ -222,7 +222,7 @@ add_ancillary_data(net_socket* socket, ancillary_data_container* container,
        cmsghdr* header = (cmsghdr*)data;
 
        while (dataLen > 0) {
-               if (header->cmsg_len < sizeof(cmsghdr) || header->cmsg_len > dataLen)
+               if (header->cmsg_len < _ALIGN(sizeof(cmsghdr)) || header->cmsg_len > dataLen)
                        return B_BAD_VALUE;
 
                if (socket->first_info->add_ancillary_data == NULL)
@@ -233,8 +233,8 @@ add_ancillary_data(net_socket* socket, ancillary_data_container* container,
                if (status != B_OK)
                        return status;
 
-               dataLen -= _ALIGN(header->cmsg_len);
-               header = (cmsghdr*)((uint8*)header + _ALIGN(header->cmsg_len));
+               dataLen -= header->cmsg_len;
+               header = (cmsghdr*)((uint8*)header + header->cmsg_len);
        }
 

Though it might be more correct to change dataLen to a ssize_t.

Opinions?

comment:2 Changed 3 years ago by korli

Blocking: 12736 added

comment:3 Changed 3 years ago by axeld

Nice catch. I think the code would look better if you replaced the if with:

if (header->cmsg_len < CMSG_LEN(0) || header->cmsg_len > datalen)

comment:4 Changed 3 years ago by axeld

Oh, about the ssize_t: I think it's fine the way it is now. The if-clause makes sure that dataLen cannot drop below zero, and you don't want to accept negative values there, anyway.

comment:5 Changed 3 years ago by korli

Thinking of it, I removed _ALIGN(), but I don't even know whether it's relevant, it might make sense to keep aligning the buffer. dataLen is actually overflown, we just need to check whether dataLen is big enough before doing the substract. Also it might make sense to check if dataLen is at least big enough to hold a header (sizeof(cmsghdr)).

 static status_t
 add_ancillary_data(net_socket* socket, ancillary_data_container* container,
        void* data, size_t dataLen)
 {
        cmsghdr* header = (cmsghdr*)data;
 
-       while (dataLen > 0) {
-               if (header->cmsg_len < sizeof(cmsghdr) || header->cmsg_len > dataLen)
+       if (dataLen == 0)
+               return B_OK;
+
+       while (true) {
+               if (header->cmsg_len < CMSG_LEN(0) || header->cmsg_len > dataLen)
                        return B_BAD_VALUE;
 
                if (socket->first_info->add_ancillary_data == NULL)
                        return B_NOT_SUPPORTED;
 
                status_t status = socket->first_info->add_ancillary_data(
                        socket->first_protocol, container, header);
                if (status != B_OK)
                        return status;
 
+               if (dataLen <= _ALIGN(header->cmsg_len))
+                       break;
                dataLen -= _ALIGN(header->cmsg_len);
                header = (cmsghdr*)((uint8*)header + _ALIGN(header->cmsg_len));
        }
 
        return B_OK;
  }

comment:6 Changed 3 years ago by axeld

Ah, I misunderstood before. You're right, the CMSG_NXTHDR() macro definition indicates that the _ALIGN() in this case is actually needed.

BTW, when you touch that method anyway, I would move the test for the availability of the add_anillary_data function in the protocol out of the loop.

comment:7 Changed 3 years ago by korli

Resolution: fixed
Status: newclosed

Fixed in hrev50266.

Note: See TracTickets for help on using tickets.