Permalink
Browse files

Fix recvfrom syscall wrapper to properly handle sockaddrs that are bi…

…gger than 'struct sockaddr'

This was a tough bug. The bug was being triggered when recvfrom was called
with a sockaddr_un out-parameter; the overflowing addr paramter would corrupt
the syscallbuf data containing the received payload. This was trigged by Samba tests,
which redirect UDP socket calls to use Unix sockets instead.
  • Loading branch information...
rocallahan committed Mar 8, 2016
1 parent 9e4f407 commit 6db59da5e08951e567d046c11b165d159a43f45f
Showing with 76 additions and 3 deletions.
  1. +1 −0 CMakeLists.txt
  2. +13 −3 src/preload/preload.c
  3. +62 −0 src/test/recvfrom.c
View
@@ -465,6 +465,7 @@ set(BASIC_TESTS
readlink
readlinkat
readv
+ recvfrom
rename
rlimit
robust_futex
View
@@ -1703,7 +1703,11 @@ static long sys_recvfrom(const struct syscall_info* call) {
void* buf = (void*)call->args[1];
size_t len = call->args[2];
int flags = call->args[3];
- struct sockaddr* src_addr = (struct sockaddr*)call->args[4];
+ /* struct sockaddr isn't useful here since some sockaddrs are bigger than
+ * it. To avoid making false assumptions, treat the sockaddr parameter
+ * as an untyped buffer.
+ */
+ void* src_addr = (void*)call->args[4];
socklen_t* addrlen = (socklen_t*)call->args[5];
void* ptr = prep_syscall_for_fd(sockfd);
@@ -1713,10 +1717,12 @@ static long sys_recvfrom(const struct syscall_info* call) {
long ret;
assert(syscallno == call->no);
+ /* If addrlen is NULL then src_addr must also be null */
+ assert(addrlen || !src_addr);
if (src_addr) {
src_addr2 = ptr;
- ptr += sizeof(*src_addr);
+ ptr += *addrlen;
}
if (addrlen) {
addrlen2 = ptr;
@@ -1737,7 +1743,11 @@ static long sys_recvfrom(const struct syscall_info* call) {
if (ret >= 0) {
if (src_addr2) {
- local_memcpy(src_addr, src_addr2, sizeof(*src_addr));
+ socklen_t actual_size = *addrlen2;
+ if (actual_size > *addrlen) {
+ actual_size = *addrlen;
+ }
+ local_memcpy(src_addr, src_addr2, actual_size);
}
if (addrlen2) {
*addrlen = *addrlen2;
View
@@ -0,0 +1,62 @@
+/* -*- Mode: C; tab-width: 8; c-basic-offset: 2; indent-tabs-mode: nil; -*- */
+
+#include "rrutil.h"
+
+static const char buf[] = "0123456789abcdefghijklmnopqrstuvwxyz";
+
+/* Make it longer than sizeof(struct sockaddr) */
+static const char sock_name[] = "sock_sock_sock_sock";
+static const char sock_name2[] = "sock_sock_sock_sock_2";
+
+int main(void) {
+ struct sockaddr_un addr;
+ struct sockaddr_un addr2;
+ struct sockaddr_un out_addr;
+ socklen_t out_addr_len;
+ char out[100];
+ int src;
+ int dest = socket(AF_UNIX, SOCK_DGRAM, 0);
+ test_assert(dest >= 0);
+
+ addr.sun_family = AF_UNIX;
+ strcpy(addr.sun_path, sock_name);
+ test_assert(0 == bind(dest, &addr, sizeof(addr)));
+
+ src = socket(AF_UNIX, SOCK_DGRAM, 0);
+ test_assert(src >= 0);
+
+ test_assert(36 == sendto(src, buf, 36, 0, &addr, sizeof(addr)));
+ out_addr_len = sizeof(out_addr);
+ test_assert(36 ==
+ recvfrom(dest, out, sizeof(out), 0, &out_addr, &out_addr_len));
+ test_assert(0 == memcmp(buf, out, 36));
+ test_assert(0 == out_addr_len);
+
+ addr2.sun_family = AF_UNIX;
+ strcpy(addr2.sun_path, sock_name2);
+ test_assert(0 == bind(src, &addr2, sizeof(addr2)));
+
+ test_assert(36 == sendto(src, buf, 36, 0, &addr, sizeof(addr)));
+ out_addr_len = sizeof(out_addr);
+ test_assert(36 ==
+ recvfrom(dest, out, sizeof(out), 0, &out_addr, &out_addr_len));
+ test_assert(0 == memcmp(buf, out, 36));
+ test_assert(out_addr_len == 2 + sizeof(sock_name2));
+ test_assert(0 == memcmp(out_addr.sun_path, sock_name2, sizeof(sock_name2)));
+
+ test_assert(36 == sendto(src, buf, 36, 0, &addr, sizeof(addr)));
+ out_addr_len = 8;
+ out_addr.sun_path[9] = 'x';
+ test_assert(36 ==
+ recvfrom(dest, out, sizeof(out), 0, &out_addr, &out_addr_len));
+ test_assert(0 == memcmp(buf, out, 36));
+ test_assert(out_addr_len == 2 + sizeof(sock_name2));
+ test_assert(0 == memcmp(out_addr.sun_path, sock_name2, 8));
+ test_assert(out_addr.sun_path[9] == 'x');
+
+ test_assert(0 == unlink(sock_name));
+ test_assert(0 == unlink(sock_name2));
+
+ atomic_puts("EXIT-SUCCESS");
+ return 0;
+}

0 comments on commit 6db59da

Please sign in to comment.