Skip to content

Commit

Permalink
rpc: fix undefined behaviour in __builtin_ctz
Browse files Browse the repository at this point in the history
Found with GCC UBsan:

rpcsvc.c:102:36: runtime error: passing zero to ctz(), which is not a valid argument
    #0 0x7fcd1ff6faa4 in rpcsvc_get_free_queue_index /path/to/glusterfs/rpc/rpc-lib/src/rpcsvc.c:102
    #1 0x7fcd1ff81e12 in rpcsvc_handle_rpc_call /path/to/glusterfs/rpc/rpc-lib/src/rpcsvc.c:837
    #2 0x7fcd1ff833ad in rpcsvc_notify /path/to/glusterfs/rpc/rpc-lib/src/rpcsvc.c:1000
    #3 0x7fcd1ff8829d in rpc_transport_notify /path/to/glusterfs/rpc/rpc-lib/src/rpc-transport.c:520
    #4 0x7fcd0dd72f16 in socket_event_poll_in_async /path/to/glusterfs/rpc/rpc-transport/socket/src/socket.c:2502
    #5 0x7fcd0dd8986a in gf_async ../../../../libglusterfs/src/glusterfs/async.h:189
    #6 0x7fcd0dd8986a in socket_event_poll_in /path/to/glusterfs/rpc/rpc-transport/socket/src/socket.c:2543
    #7 0x7fcd0dd8986a in socket_event_handler /path/to/glusterfs/rpc/rpc-transport/socket/src/socket.c:2934
    #8 0x7fcd0dd8986a in socket_event_handler /path/to/glusterfs/rpc/rpc-transport/socket/src/socket.c:2854
    #9 0x7fcd2048aff7 in event_dispatch_epoll_handler /path/to/glusterfs/libglusterfs/src/event-epoll.c:640
    #10 0x7fcd2048aff7 in event_dispatch_epoll_worker /path/to/glusterfs/libglusterfs/src/event-epoll.c:751
    ...

Fix, simplify, and prefer 'unsigned long' as underlying bitmap type.

Change-Id: If3f24dfe7bef8bc7a11a679366e219a73caeb9e4
Signed-off-by: Dmitry Antipov <[email protected]>
Fixes: #1283
  • Loading branch information
dmantipov authored and amarts committed Jun 22, 2020
1 parent a349c39 commit ab2fe47
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 41 deletions.
19 changes: 19 additions & 0 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -911,6 +911,25 @@ if test "x${have_backtrace}" = "xyes"; then
fi
AC_SUBST(HAVE_BACKTRACE)

dnl Old (before C11) compiler can compile (but not link) this:
dnl
dnl int main () {
dnl _Static_assert(1, "True");
dnl return 0;
dnl }
dnl
dnl assuming that _Static_assert is an implicitly declared function. So
dnl we're trying to link just to make sure that this is not the case.

AC_MSG_CHECKING([whether $CC supports C11 _Static_assert])
AC_TRY_LINK([], [_Static_assert(1, "True");],
[STATIC_ASSERT=yes], [STATIC_ASSERT=no])

AC_MSG_RESULT([$STATIC_ASSERT])
if test x$STATIC_ASSERT = "xyes"; then
AC_DEFINE(HAVE_STATIC_ASSERT, 1, [Define if C11 _Static_assert is supported.])
fi

if test "x${have_backtrace}" != "xyes"; then
AC_TRY_COMPILE([#include <math.h>], [double x=0.0; x=ceil(0.0);],
[],
Expand Down
15 changes: 15 additions & 0 deletions libglusterfs/src/glusterfs/common-utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <string.h>
#include <assert.h>
#include <pthread.h>
#include <unistd.h>
#include <openssl/md5.h>
#ifndef GF_BSD_HOST_OS
#include <alloca.h>
Expand All @@ -26,6 +27,11 @@
#include <fnmatch.h>
#include <uuid/uuid.h>

/* FreeBSD, etc. */
#ifndef __BITS_PER_LONG
#define __BITS_PER_LONG (CHAR_BIT * (sizeof(long)))
#endif

#ifndef ffsll
#define ffsll(x) __builtin_ffsll(x)
#endif
Expand Down Expand Up @@ -441,6 +447,15 @@ BIT_VALUE(unsigned char *array, unsigned int index)
} while (0)
#endif

/* Compile-time assert, borrowed from Linux kernel. */
#ifdef HAVE_STATIC_ASSERT
#define GF_STATIC_ASSERT(expr, ...) \
__gf_static_assert(expr, ##__VA_ARGS__, #expr)
#define __gf_static_assert(expr, msg, ...) _Static_assert(expr, msg)
#else
#define GF_STATIC_ASSERT(expr, ...)
#endif

#define GF_ABORT(msg...) \
do { \
gf_msg_callingfn("", GF_LOG_CRITICAL, 0, LG_MSG_ASSERTION_FAILED, \
Expand Down
4 changes: 4 additions & 0 deletions libglusterfs/src/glusterfs/gf-event.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#define _GF_EVENT_H_

#include <pthread.h>
#include "common-utils.h"
#include "list.h"

struct event_pool;
Expand All @@ -31,6 +32,9 @@ typedef void (*event_handler_t)(int fd, int idx, int gen, void *data,
#define EVENT_EPOLL_SLOTS 1024
#define EVENT_MAX_THREADS 1024

/* See rpcsvc.h to check why. */
GF_STATIC_ASSERT(EVENT_MAX_THREADS % __BITS_PER_LONG == 0);

struct event_pool {
struct event_ops *ops;

Expand Down
54 changes: 14 additions & 40 deletions rpc/rpc-lib/src/rpcsvc.c
Original file line number Diff line number Diff line change
Expand Up @@ -66,59 +66,33 @@ rpcsvc_request_handler(void *arg);
static int
rpcsvc_match_subnet_v4(const char *addrtok, const char *ipaddr);

void
static void
rpcsvc_toggle_queue_status(rpcsvc_program_t *prog,
rpcsvc_request_queue_t *queue, char status[])
rpcsvc_request_queue_t *queue,
unsigned long status[])
{
int queue_index = 0, status_index = 0, set_bit = 0;

if (queue != &prog->request_queue[0]) {
queue_index = (queue - &prog->request_queue[0]);
}

status_index = queue_index / 8;
set_bit = queue_index % 8;
unsigned queue_index = queue - prog->request_queue;

status[status_index] ^= (1 << set_bit);

return;
status[queue_index / __BITS_PER_LONG] ^= (1UL << (queue_index %
__BITS_PER_LONG));
}

int
rpcsvc_get_free_queue_index(rpcsvc_program_t *prog)
{
int queue_index = 0, max_index = 0, i = 0;
unsigned int right_most_unset_bit = 0;
unsigned i, j = 0;

right_most_unset_bit = 8;

max_index = gf_roof(EVENT_MAX_THREADS, 8) / 8;
for (i = 0; i < max_index; i++) {
if (prog->request_queue_status[i] == 0) {
right_most_unset_bit = 0;
for (i = 0; i < EVENT_MAX_THREADS / __BITS_PER_LONG; i++)
if (prog->request_queue_status[i] != ULONG_MAX) {
j = __builtin_ctzl(~prog->request_queue_status[i]);
break;
} else {
/* get_rightmost_set_bit (sic)*/
right_most_unset_bit = __builtin_ctz(
~prog->request_queue_status[i]);
if (right_most_unset_bit < 8) {
break;
}
}
}

if (right_most_unset_bit > 7) {
queue_index = -1;
} else {
queue_index = i * 8;
queue_index += right_most_unset_bit;
}

if (queue_index != -1) {
prog->request_queue_status[i] |= (0x1 << right_most_unset_bit);
}
if (i == EVENT_MAX_THREADS / __BITS_PER_LONG)
return -1;

return queue_index;
prog->request_queue_status[i] |= (1UL << j);
return i * __BITS_PER_LONG + j;
}

rpcsvc_notify_wrapper_t *
Expand Down
2 changes: 1 addition & 1 deletion rpc/rpc-lib/src/rpcsvc.h
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ struct rpcsvc_program {
gf_boolean_t alive;

gf_boolean_t synctask;
char request_queue_status[EVENT_MAX_THREADS / 8 + 1];
unsigned long request_queue_status[EVENT_MAX_THREADS / __BITS_PER_LONG];
};

typedef struct rpcsvc_cbk_program {
Expand Down

0 comments on commit ab2fe47

Please sign in to comment.