WASI sockets: POSIX platform os_socket_recvfrom unintended behavior

[lucasAbadFr]

Subject of the issue

When increasing the support for zephyr and comparing comportement between zephyr and linux, I found out with my sample that the function recvfrom might have a problem.

Test case

Bellow is the simplified (removed includes, checks, prints, ...) code that I used to see the problem with recvfrom:

#define SSTRLEN(s) (sizeof(s) - 1)
#define REQUEST "GET / HTTP/1.0\r\nHost: 127.0.0.1\r\n\r\n"

static char response[1024];

int
main(int argc, char **argv)
{
    int st, sock;
    struct sockaddr_in addr;
    int rc = 0;

    // server address 127.0.0.1:8000
    memset(&addr, 0, sizeof(addr));
    addr.sin_family = AF_INET;
    addr.sin_port = htons(8000);
    addr.sin_addr.s_addr = htonl(2130706433); 

    sock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);

    rc = connect(sock, (struct sockaddr *)&addr, sizeof(addr));

    rc = sendto(sock, (const void *)REQUEST, SSTRLEN(REQUEST), 0,
                (struct sockaddr *)&addr, sizeof(addr));
    
    socklen_t socklen = sizeof(addr);
    
    while (1) {
        int len = recvfrom(sock, response, sizeof(response) - 1, 0,
                           (struct sockaddr *)&addr, &socklen);

        if (len < 0) {
            return 0;
        }

        response[len] = 0;
        printf("%s", response);

        if (len == 0) {
            break;
        }
    }
    printf("\n");

    (void)close(sock);
    return 0;
}

Your environment

• Host OS: Ubuntu 22.04.4 LTS
• WAMR version: Any
• platform: Linux / POSIX
• cpu architecture: x86_64
• running mode: default

Steps to reproduce

Compile the code to wasm using the socket sample or standalone (need to link it with libwasi_socket_ext.a) and run it with command:

./iwasm --addr-pool=127.0.0.1/24 http_get.wasm

Expected behavior

Run without error.

Actual behavior

Fail after receiving first part of the data.

Extra Info

The code above is a simple client that sends a request to a server and waits for the response. The server is a simple server that sends a response to the client. The server used is the following:

python -m http.server --bind 0.0.0.0

My first observation was that strangely, when commenting the else if statement in os_socket_recv_from the code worked as expected.

wasm-micro-runtime/core/shared/platform/common/posix/posix_socket.c

Lines 258 to 284 in 67dce48

	int
	os_socket_recv_from(bh_socket_t socket, void *buf, unsigned int len, int flags,
	bh_sockaddr_t *src_addr)
	{
	struct sockaddr_storage sock_addr = { 0 };
	socklen_t socklen = sizeof(sock_addr);
	int ret;
	ret = recvfrom(socket, buf, len, flags, (struct sockaddr *)&sock_addr,
	&socklen);
	if (ret < 0) {
	return ret;
	}
	if (src_addr && socklen > 0) {
	if (sockaddr_to_bh_sockaddr((struct sockaddr *)&sock_addr, src_addr)
	== BHT_ERROR) {
	return -1;
	}
	}
	else if (src_addr) {
	memset(src_addr, 0, sizeof(*src_addr));
	}
	return ret;
	}

Firstly, I know that recvfrom is mostly used for UDP and not TCP, but I wanted to test the behavior of the function in wasm, because in native code there is no problem using the function this way. Even in the man page we can read: "The recvfrom() and recvmsg() calls are used to receive messages from a socket, and may be used to receive data on a socket whether or not it is connection-oriented."

This is the same for sendto and send, but I will focus on recvfrom because it is the one that is causing the problem and the one I investigated.

In the following statements, I will use POSIX name to refer to the os_socket_* functions.

Secondly, the implementation of the recv function simply call the recvfrom function, by passing an additionnal uninitialized parameter src_addr (of type __wasi_addr_t). See the implementation :

Secondly, the implementation of the recv function simply call the recvfrom function, by passing an additionnal uninitialized parameter src_addr (of type __wasi_addr_t). See the implementation:

https://github.com/bytecodealliance/wasm-micro-runtime/blob/67dce482018bc3103f02018d9682dcbb5154661f/core/iwasm/libraries/libc-wasi/sandboxed-system-primitives/src/posix.c#L2810:L2819

Finally, I tested the code with the recv function and it worked fine, but I wanted to know why the recvfrom function is not working as expected.

As we use TCP, the addrlen parameter is set to 0 and we keep passing in the else if because src_addr is not NULL. The problem is that we will overwrite the src_addr parameter with memset.

If we are lucky and we can receive the message in one buffer we will not have any problem, but if the message is bigger than the buffer we will have a problem because the src_addr parameter will be overwritten and the function will return an error after the next call.

The problem doesn't show with recv because the src_addr is not coming from the user, but from the implementation, however with recvfrom the src_addr is coming from the user.

I'm not sure if this case is known or intended, but I think we are introducing a buggy behavior, because it doesn't work as expected (like in native).

[wenyongh]

@lucasAbadFr thanks for reporting the issue!

@yamt and @loganek could you help have a look? Seems we had better remove these three lines in posix os_socket_recv_from?

wasm-micro-runtime/core/shared/platform/common/posix/posix_socket.c

Lines 279 to 281 in e8c2952

	else if (src_addr) {
	memset(src_addr, 0, sizeof(*src_addr));
	}

[yamt]

@lucasAbadFr thanks for reporting the issue!

@yamt and @loganek could you help have a look? Seems we had better remove these three lines in posix os_socket_recv_from?

why are you asking us?
i have not worked on the wamr-specific socket stuff at all.

wasm-micro-runtime/core/shared/platform/common/posix/posix_socket.c

Lines 279 to 281 in e8c2952

	else if (src_addr) {
	memset(src_addr, 0, sizeof(*src_addr));
	}

at a glance, it seems broken.
but removing the memset doesn't seem like a correct fix either.
i guess we need a way to report "no address is available".
maybe we can tweak __wasi_addr_t and bh_sockaddr_t to be able to represent the situation.

[wenyongh]

@lucasAbadFr thanks for reporting the issue!
@yamt and @loganek could you help have a look? Seems we had better remove these three lines in posix os_socket_recv_from?

why are you asking us? i have not worked on the wamr-specific socket stuff at all.

Sorry that I just think you are warmhearted and experienced😊, and may have good advice on this issue.

[yamt]

@lucasAbadFr thanks for reporting the issue!
@yamt and @loganek could you help have a look? Seems we had better remove these three lines in posix os_socket_recv_from?

why are you asking us? i have not worked on the wamr-specific socket stuff at all.

Sorry that I just think you are warmhearted and experienced😊, and may have good advice on this issue.

heh, ok i got it.

[loganek]

yeah I agree this seems like incorrect behavior. @lucasAbadFr would you like to provide a fix for it? If not, I might have some time next week to address the issue.

[lucasAbadFr]

yeah I agree this seems like incorrect behavior. @lucasAbadFr would you like to provide a fix for it? If not, I might have some time next week to address the issue.

Hi @loganek, I will not have that much free time in the next weeks. If you can provide a fix it would appreciated.

I think that, we should just move this specific case from os_socket_recvfrom to wasmtime_ssp_sock_recv

__wasi_errno_t
wasmtime_ssp_sock_recv(wasm_exec_env_t exec_env, struct fd_table *curfds,
                       __wasi_fd_t sock, void *buf, size_t buf_len,
                       size_t *recv_len)
{
    __wasi_addr_t src_addr;
    __wasi_errno_t error = -1;

    error = wasmtime_ssp_sock_recv_from(exec_env, curfds, sock, buf, buf_len, 0,
                                       &src_addr, recv_len);

    if(src_addr && *recv_len > 0) {
        /* Zero out the memory of `src_addr` after call to `recvfrom` */
        memset(src_addr, 0, sizeof(*src_addr));
    }

    return error
}

[lum1n0us]

/* Zero out the memory of src_addr after call to recvfrom */

not sure about that. since src_addr is a local variable, zero out or not may not change outputs of wasmtime_ssp_sock_recv().