From 7f92df961d7db4bc1881e8bc38184192ba90c0d6 Mon Sep 17 00:00:00 2001 From: logwang Date: Tue, 5 Dec 2017 15:32:10 +0800 Subject: [PATCH 1/3] Fix #114: An out of bounds of memory in netinet/libalias/alias_sctp.c. Run with valgrind, and found this: ==2228== Invalid write of size 8 ==2228== at 0x4E05DA: AliasSctpInit (alias_sctp.c:641) ==2228== by 0x4DE565: LibAliasInit (alias_db.c:2503) ==2228== by 0x4E9B3B: nat44_config (ip_fw_nat.c:505) ==2228== by 0x4E9E91: nat44_cfg (ip_fw_nat.c:599) ==2228== by 0x4F1719: ipfw_ctl3 (ip_fw_sockopt.c:3666) ==2228== by 0x4B9954: rip_ctloutput (raw_ip.c:659) ==2228== by 0x447E11: sosetopt (uipc_socket.c:2505) ==2228== by 0x44BF4D: kern_setsockopt (uipc_syscalls.c:1407) ==2228== by 0x409F08: ff_setsockopt (ff_syscall_wrapper.c:412) ==2228== by 0x5277AA: handle_ipfw_msg (ff_dpdk_if.c:1146) ==2228== by 0x52788C: handle_msg (ff_dpdk_if.c:1196) ==2228== by 0x5289B8: process_msg_ring (ff_dpdk_if.c:1213) ==2228== Address 0x60779b0 is 4,800 bytes inside a block of size 4,802 alloc'd ==2228== at 0x4C2ABBD: malloc (vg_replace_malloc.c:296) ==2228== by 0x509F15: ff_malloc (ff_host_interface.c:89) ==2228== by 0x4053BE: malloc (ff_glue.c:1021) ==2228== by 0x4E054E: AliasSctpInit (alias_sctp.c:632) ==2228== by 0x4DE565: LibAliasInit (alias_db.c:2503) ==2228== by 0x4E9B3B: nat44_config (ip_fw_nat.c:505) ==2228== by 0x4E9E91: nat44_cfg (ip_fw_nat.c:599) ==2228== by 0x4F1719: ipfw_ctl3 (ip_fw_sockopt.c:3666) ==2228== by 0x4B9954: rip_ctloutput (raw_ip.c:659) ==2228== by 0x447E11: sosetopt (uipc_socket.c:2505) ==2228== by 0x44BF4D: kern_setsockopt (uipc_syscalls.c:1407) ==2228== by 0x409F08: ff_setsockopt (ff_syscall_wrapper.c:412) ==2228== The error line is: `la->sctpNatTimer.TimerQ = sn_calloc(SN_TIMER_QUEUE_SIZE, sizeof(struct sctpTimerQ));` Since SN_TIMER_QUEUE_SIZE is defined as SN_MAX_TIMER+2, and sn_calloc is defined as sn_malloc(x * n) if _SYS_MALLOC_H_ is defined, the size of calloced memory will be wrong, because the macro will be expanded to sizeof(struct sctpTimerQ)*SN_MAX_TIMER+2. And the memory will be out of bounds here. ``` /* Initialise circular timer Q*/ for (i = 0; i < SN_TIMER_QUEUE_SIZE; i++) LIST_INIT(&la->sctpNatTimer.TimerQ[i]); ``` --- freebsd/netinet/libalias/alias_sctp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/freebsd/netinet/libalias/alias_sctp.c b/freebsd/netinet/libalias/alias_sctp.c index 67e475402..08d8b4d78 100644 --- a/freebsd/netinet/libalias/alias_sctp.c +++ b/freebsd/netinet/libalias/alias_sctp.c @@ -185,7 +185,7 @@ static MALLOC_DEFINE(M_SCTPNAT, "sctpnat", "sctp nat dbs"); /* Use kernel allocator. */ #ifdef _SYS_MALLOC_H_ #define sn_malloc(x) malloc(x, M_SCTPNAT, M_NOWAIT|M_ZERO) -#define sn_calloc(n,x) sn_malloc(x * n) +#define sn_calloc(n,x) sn_malloc((x) * (n)) #define sn_free(x) free(x, M_SCTPNAT) #endif// #ifdef _SYS_MALLOC_H_ From 794317ab373d7d1fe243aca5b764bc0a5474d3a2 Mon Sep 17 00:00:00 2001 From: logwang Date: Tue, 5 Dec 2017 17:51:02 +0800 Subject: [PATCH 2/3] ff_epoll: support edge-triggered mode. Convert epoll EPOLLET to kqueue EV_CLEAR. --- lib/ff_epoll.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/lib/ff_epoll.c b/lib/ff_epoll.c index 3e236b8bb..e82bb89f9 100644 --- a/lib/ff_epoll.c +++ b/lib/ff_epoll.c @@ -27,7 +27,7 @@ ff_epoll_create(int size __attribute__((__unused__))) return ff_kqueue(); } -int +int ff_epoll_ctl(int epfd, int op, int fd, struct epoll_event *event) { struct kevent kev[3]; @@ -38,23 +38,33 @@ ff_epoll_ctl(int epfd, int op, int fd, struct epoll_event *event) } if (op == EPOLL_CTL_ADD){ + int flags = EV_ADD; + if (event->events & EPOLLET) { + flags |= EV_CLEAR; + } + EV_SET(&kev[0], fd, EVFILT_READ, - EV_ADD | (event->events & EPOLLIN ? 0 : EV_DISABLE), 0, 0, NULL); + flags | (event->events & EPOLLIN ? 0 : EV_DISABLE), 0, 0, NULL); EV_SET(&kev[1], fd, EVFILT_WRITE, - EV_ADD | (event->events & EPOLLOUT ? 0 : EV_DISABLE), 0, 0, NULL); + flags | (event->events & EPOLLOUT ? 0 : EV_DISABLE), 0, 0, NULL); EV_SET(&kev[2], fd, EVFILT_USER, EV_ADD, - event->events & EPOLLRDHUP ? 1 : 0, 0, NULL); + event->events & EPOLLRDHUP ? 1 : 0, 0, NULL); } else if (op == EPOLL_CTL_DEL) { EV_SET(&kev[0], fd, EVFILT_READ, EV_DELETE, 0, 0, NULL); EV_SET(&kev[1], fd, EVFILT_WRITE, EV_DELETE, 0, 0, NULL); EV_SET(&kev[2], fd, EVFILT_USER, EV_DELETE, 0, 0, NULL); } else if (op == EPOLL_CTL_MOD) { + int flags = 0; + if (event->events & EPOLLET) { + flags |= EV_CLEAR; + } + EV_SET(&kev[0], fd, EVFILT_READ, - event->events & EPOLLIN ? EV_ENABLE : EV_DISABLE, 0, 0, NULL); + flags | (event->events & EPOLLIN ? EV_ENABLE : EV_DISABLE), 0, 0, NULL); EV_SET(&kev[1], fd, EVFILT_WRITE, - event->events & EPOLLOUT ? EV_ENABLE : EV_DISABLE, 0, 0, NULL); + flags | (event->events & EPOLLOUT ? EV_ENABLE : EV_DISABLE), 0, 0, NULL); EV_SET(&kev[2], fd, EVFILT_USER, 0, - NOTE_FFCOPY | (event->events & EPOLLRDHUP ? 1 : 0), 0, NULL); + NOTE_FFCOPY | (event->events & EPOLLRDHUP ? 1 : 0), 0, NULL); } else { errno = EINVAL; return -1; From 4ca4a48737c132d4f21e35c398b4edee4dbce0a8 Mon Sep 17 00:00:00 2001 From: logwang Date: Thu, 7 Dec 2017 12:24:49 +0800 Subject: [PATCH 3/3] ff_epoll: rewrite `ff_epoll_ctl`. --- lib/ff_epoll.c | 91 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 31 deletions(-) diff --git a/lib/ff_epoll.c b/lib/ff_epoll.c index e82bb89f9..96e7a9e7e 100644 --- a/lib/ff_epoll.c +++ b/lib/ff_epoll.c @@ -30,47 +30,76 @@ ff_epoll_create(int size __attribute__((__unused__))) int ff_epoll_ctl(int epfd, int op, int fd, struct epoll_event *event) { - struct kevent kev[3]; + /* + * Since kqueue uses EVFILT_READ and EVFILT_WRITE filters to + * handle read/write events, so we need two kevents. + */ + const int changes = 2; + struct kevent kev[changes]; + int flags = 0; + int read_flags, write_flags; - if (!event && op != EPOLL_CTL_DEL) { + if ((!event && op != EPOLL_CTL_DEL) || + (op != EPOLL_CTL_ADD && + op != EPOLL_CTL_MOD && + op != EPOLL_CTL_DEL)) { errno = EINVAL; return -1; } - if (op == EPOLL_CTL_ADD){ - int flags = EV_ADD; - if (event->events & EPOLLET) { - flags |= EV_CLEAR; - } - - EV_SET(&kev[0], fd, EVFILT_READ, - flags | (event->events & EPOLLIN ? 0 : EV_DISABLE), 0, 0, NULL); - EV_SET(&kev[1], fd, EVFILT_WRITE, - flags | (event->events & EPOLLOUT ? 0 : EV_DISABLE), 0, 0, NULL); - EV_SET(&kev[2], fd, EVFILT_USER, EV_ADD, - event->events & EPOLLRDHUP ? 1 : 0, 0, NULL); - } else if (op == EPOLL_CTL_DEL) { + /* + * EPOLL_CTL_DEL doesn't need to care for event->events. + */ + if (op == EPOLL_CTL_DEL) { EV_SET(&kev[0], fd, EVFILT_READ, EV_DELETE, 0, 0, NULL); EV_SET(&kev[1], fd, EVFILT_WRITE, EV_DELETE, 0, 0, NULL); - EV_SET(&kev[2], fd, EVFILT_USER, EV_DELETE, 0, 0, NULL); - } else if (op == EPOLL_CTL_MOD) { - int flags = 0; - if (event->events & EPOLLET) { - flags |= EV_CLEAR; - } - EV_SET(&kev[0], fd, EVFILT_READ, - flags | (event->events & EPOLLIN ? EV_ENABLE : EV_DISABLE), 0, 0, NULL); - EV_SET(&kev[1], fd, EVFILT_WRITE, - flags | (event->events & EPOLLOUT ? EV_ENABLE : EV_DISABLE), 0, 0, NULL); - EV_SET(&kev[2], fd, EVFILT_USER, 0, - NOTE_FFCOPY | (event->events & EPOLLRDHUP ? 1 : 0), 0, NULL); - } else { - errno = EINVAL; - return -1; + return ff_kevent(epfd, kev, changes, NULL, 0, NULL); } - return ff_kevent(epfd, kev, 3, NULL, 0, NULL); + /* + * FIXME: + * + * Kqueue doesn't have edge-triggered mode that exactly + * same with epoll, the most similar way is setting EV_CLEAR + * or EV_DISPATCH flag, but there are still some differences. + * + * EV_CLEAR:after the event is retrieved by the user, + * its state is reset. + * EV_DISPATCH: disable the event source immediately + * after delivery of an event. + * + * Here we use EV_CLEAR temporarily. + * + */ + if (event->events & EPOLLET) { + flags |= EV_CLEAR; + } + + if (event->events & EPOLLONESHOT) { + flags |= EV_ONESHOT; + } + + if (op == EPOLL_CTL_ADD) { + flags |= EV_ADD; + } + + read_flags = write_flags = flags | EV_DISABLE; + + if (event->events & EPOLLIN) { + read_flags &= ~EV_DISABLE; + read_flags |= EV_ENABLE; + } + + if (event->events & EPOLLOUT) { + write_flags &= ~EV_DISABLE; + write_flags |= EV_ENABLE; + } + + EV_SET(&kev[0], fd, EVFILT_READ, read_flags, 0, 0, NULL); + EV_SET(&kev[1], fd, EVFILT_WRITE, write_flags, 0, 0, NULL); + + return ff_kevent(epfd, kev, changes, NULL, 0, NULL); } static void