Skip to content

Commit

Permalink
[LibOS] Get rid of FDTYPE and replace it with uint32_t
Browse files Browse the repository at this point in the history 
Previously, LibOS used `typedef uint16_t FDTYPE`. This was unnecessary
and hard-to-read. Moreover, it led to a bug in `__enlarge_handle_map()`:
the new size was of 64-bit type `size_t` but it was assigned to
`fd_size` field of 16-bit type `FDTYPE`, which led to silent truncation.
This resulted in failing asserts, because active FDs suddenly became
greater than the limit in the FD map.

This commit fixes this bug by changing the type from 16-bit to 32-bit,
and also removes the opaque and confusing `FDTYPE`.

Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
  • Loading branch information
@dimakuv
dimakuv committed Sep 14, 2021
1 parent fce7a6b commit af787cd
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 40 deletions.
22 changes: 11 additions & 11 deletions LibOS/shim/include/shim_handle.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ struct shim_tmpfs_handle {
DEFINE_LIST(shim_epoll_item);
DEFINE_LISTP(shim_epoll_item);
struct shim_epoll_item {
FDTYPE fd;
int fd;
uint64_t data;
unsigned int events;
unsigned int revents;
Expand Down Expand Up @@ -259,16 +259,16 @@ int set_handle_nonblocking(struct shim_handle* hdl, bool on);

/* file descriptor table */
struct shim_fd_handle {
FDTYPE vfd; /* virtual file descriptor */
int flags; /* file descriptor flags, only FD_CLOEXEC */
uint32_t vfd; /* virtual file descriptor */
int flags; /* file descriptor flags, only FD_CLOEXEC */

struct shim_handle* handle;
};

struct shim_handle_map {
/* the top of created file descriptors */
FDTYPE fd_size;
FDTYPE fd_top;
uint32_t fd_size;
uint32_t fd_top;

/* refrence count and lock */
REFTYPE ref_count;
Expand All @@ -279,11 +279,11 @@ struct shim_handle_map {
};

/* allocating file descriptors */
#define FD_NULL ((FDTYPE)-1)
#define FD_NULL UINT32_MAX
#define HANDLE_ALLOCATED(fd_handle) ((fd_handle) && (fd_handle)->vfd != FD_NULL)

struct shim_handle* __get_fd_handle(FDTYPE fd, int* flags, struct shim_handle_map* map);
struct shim_handle* get_fd_handle(FDTYPE fd, int* flags, struct shim_handle_map* map);
struct shim_handle* __get_fd_handle(uint32_t fd, int* flags, struct shim_handle_map* map);
struct shim_handle* get_fd_handle(uint32_t fd, int* flags, struct shim_handle_map* map);

/*!
* \brief Assign new fd to a handle.
Expand All @@ -296,13 +296,13 @@ struct shim_handle* get_fd_handle(FDTYPE fd, int* flags, struct shim_handle_map*
* Uses the lowest, non-negative available number for the new fd.
*/
int set_new_fd_handle(struct shim_handle* hdl, int fd_flags, struct shim_handle_map* map);
int set_new_fd_handle_by_fd(FDTYPE fd, struct shim_handle* hdl, int fd_flags,
int set_new_fd_handle_by_fd(uint32_t fd, struct shim_handle* hdl, int fd_flags,
struct shim_handle_map* map);
int set_new_fd_handle_above_fd(FDTYPE fd, struct shim_handle* hdl, int fd_flags,
int set_new_fd_handle_above_fd(uint32_t fd, struct shim_handle* hdl, int fd_flags,
struct shim_handle_map* map);
struct shim_handle* __detach_fd_handle(struct shim_fd_handle* fd, int* flags,
struct shim_handle_map* map);
struct shim_handle* detach_fd_handle(FDTYPE fd, int* flags, struct shim_handle_map* map);
struct shim_handle* detach_fd_handle(uint32_t fd, int* flags, struct shim_handle_map* map);

/* manage handle mapping */
int dup_handle_map(struct shim_handle_map** new_map, struct shim_handle_map* old_map);
Expand Down
2 changes: 0 additions & 2 deletions LibOS/shim/include/shim_types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,6 @@ typedef Elf64_auxv_t elf_auxv_t;
/* typedef for shim internal types */
typedef uint32_t IDTYPE;
#define IDTYPE_MAX UINT32_MAX
typedef uint16_t FDTYPE;
#define FDTYPE_MAX UINT16_MAX
typedef uint64_t HASHTYPE;

#define FILE_OFF_MAX INT64_MAX
Expand Down
57 changes: 33 additions & 24 deletions LibOS/shim/src/bookkeep/shim_handle.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,12 @@ static int init_exec_handle(void) {
return ret;
}

static struct shim_handle_map* get_new_handle_map(FDTYPE size);
static struct shim_handle_map* get_new_handle_map(uint32_t size);

static int __init_handle(struct shim_fd_handle** fdhdl, FDTYPE fd, struct shim_handle* hdl,
static int __init_handle(struct shim_fd_handle** fdhdl, uint32_t fd, struct shim_handle* hdl,
int fd_flags);

static int __enlarge_handle_map(struct shim_handle_map* map, size_t size);
static int __enlarge_handle_map(struct shim_handle_map* map, uint32_t size);

int init_handle(void) {
if (!create_lock(&handle_mgr_lock)) {
Expand Down Expand Up @@ -246,7 +246,7 @@ int init_important_handles(void) {
return init_exec_handle();
}

struct shim_handle* __get_fd_handle(FDTYPE fd, int* fd_flags, struct shim_handle_map* map) {
struct shim_handle* __get_fd_handle(uint32_t fd, int* fd_flags, struct shim_handle_map* map) {
assert(locked(&map->lock));

struct shim_fd_handle* fd_handle = NULL;
Expand All @@ -264,7 +264,7 @@ struct shim_handle* __get_fd_handle(FDTYPE fd, int* fd_flags, struct shim_handle
return NULL;
}

struct shim_handle* get_fd_handle(FDTYPE fd, int* fd_flags, struct shim_handle_map* map) {
struct shim_handle* get_fd_handle(uint32_t fd, int* fd_flags, struct shim_handle_map* map) {
if (!map)
map = get_thread_handle_map(NULL);

Expand All @@ -283,8 +283,8 @@ struct shim_handle* __detach_fd_handle(struct shim_fd_handle* fd, int* flags,
struct shim_handle* handle = NULL;

if (HANDLE_ALLOCATED(fd)) {
int vfd = fd->vfd;
handle = fd->handle;
uint32_t vfd = fd->vfd;
handle = fd->handle;
if (flags)
*flags = fd->flags;

Expand All @@ -294,15 +294,19 @@ struct shim_handle* __detach_fd_handle(struct shim_fd_handle* fd, int* flags,

if (vfd == map->fd_top)
do {
map->fd_top = vfd ? vfd - 1 : FD_NULL;
if (vfd == 0) {
map->fd_top = FD_NULL;
break;
}
map->fd_top = vfd - 1;
vfd--;
} while (vfd >= 0 && !HANDLE_ALLOCATED(map->map[vfd]));
} while (!HANDLE_ALLOCATED(map->map[vfd]));
}

return handle;
}

struct shim_handle* detach_fd_handle(FDTYPE fd, int* flags, struct shim_handle_map* handle_map) {
struct shim_handle* detach_fd_handle(uint32_t fd, int* flags, struct shim_handle_map* handle_map) {
struct shim_handle* handle = NULL;

if (!handle_map && !(handle_map = get_thread_handle_map(NULL)))
Expand Down Expand Up @@ -349,7 +353,7 @@ struct shim_handle* get_new_handle(void) {
return new_handle;
}

static int __init_handle(struct shim_fd_handle** fdhdl, FDTYPE fd, struct shim_handle* hdl,
static int __init_handle(struct shim_fd_handle** fdhdl, uint32_t fd, struct shim_handle* hdl,
int fd_flags) {
struct shim_fd_handle* new_handle = *fdhdl;
assert((fd_flags & ~FD_CLOEXEC) == 0); // The only supported flag right now
Expand All @@ -372,7 +376,7 @@ static int __init_handle(struct shim_fd_handle** fdhdl, FDTYPE fd, struct shim_h
* Helper function for set_new_fd_handle*(). If find_free is true, tries to find the first free fd
* (starting from the provided one), otherwise, tries to use fd as-is.
*/
static int __set_new_fd_handle(FDTYPE fd, struct shim_handle* hdl, int fd_flags,
static int __set_new_fd_handle(uint32_t fd, struct shim_handle* hdl, int fd_flags,
struct shim_handle_map* handle_map, bool find_free) {
int ret;

Expand Down Expand Up @@ -404,11 +408,16 @@ static int __set_new_fd_handle(FDTYPE fd, struct shim_handle* hdl, int fd_flags,

// Enlarge handle_map->map (or allocate if necessary)
if (fd >= handle_map->fd_size) {
size_t new_size = handle_map->fd_size;
uint32_t new_size = handle_map->fd_size;
if (new_size == 0)
new_size = INIT_HANDLE_MAP_SIZE;
while (new_size <= fd)
new_size *= 2;

while (new_size <= fd) {
if (__builtin_mul_overflow(new_size, 2, &new_size)) {
ret = -ENFILE;
goto out;
}
}

ret = __enlarge_handle_map(handle_map, new_size);
if (ret < 0)
Expand All @@ -435,12 +444,12 @@ int set_new_fd_handle(struct shim_handle* hdl, int fd_flags, struct shim_handle_
return __set_new_fd_handle(0, hdl, fd_flags, handle_map, /*find_first=*/true);
}

int set_new_fd_handle_by_fd(FDTYPE fd, struct shim_handle* hdl, int fd_flags,
int set_new_fd_handle_by_fd(uint32_t fd, struct shim_handle* hdl, int fd_flags,
struct shim_handle_map* handle_map) {
return __set_new_fd_handle(fd, hdl, fd_flags, handle_map, /*find_first=*/false);
}

int set_new_fd_handle_above_fd(FDTYPE fd, struct shim_handle* hdl, int fd_flags,
int set_new_fd_handle_above_fd(uint32_t fd, struct shim_handle* hdl, int fd_flags,
struct shim_handle_map* handle_map) {
return __set_new_fd_handle(fd, hdl, fd_flags, handle_map, /*find_first=*/true);
}
Expand Down Expand Up @@ -537,7 +546,7 @@ int get_file_size(struct shim_handle* hdl, uint64_t* size) {
return 0;
}

static struct shim_handle_map* get_new_handle_map(FDTYPE size) {
static struct shim_handle_map* get_new_handle_map(uint32_t size) {
struct shim_handle_map* handle_map = calloc(1, sizeof(struct shim_handle_map));

if (!handle_map)
Expand All @@ -563,7 +572,7 @@ static struct shim_handle_map* get_new_handle_map(FDTYPE size) {
return handle_map;
}

static int __enlarge_handle_map(struct shim_handle_map* map, size_t size) {
static int __enlarge_handle_map(struct shim_handle_map* map, uint32_t size) {
assert(locked(&map->lock));

if (size <= map->fd_size)
Expand Down Expand Up @@ -595,7 +604,7 @@ int dup_handle_map(struct shim_handle_map** new, struct shim_handle_map* old_map
if (old_map->fd_top == FD_NULL)
goto done;

for (int i = 0; i <= old_map->fd_top; i++) {
for (uint32_t i = 0; i <= old_map->fd_top; i++) {
struct shim_fd_handle* fd_old = old_map->map[i];
struct shim_fd_handle* fd_new;

Expand All @@ -608,7 +617,7 @@ int dup_handle_map(struct shim_handle_map** new, struct shim_handle_map* old_map

fd_new = malloc(sizeof(struct shim_fd_handle));
if (!fd_new) {
for (int j = 0; j < i; j++) {
for (uint32_t j = 0; j < i; j++) {
put_handle(new_map->map[j]->handle);
free(new_map->map[j]);
}
Expand Down Expand Up @@ -643,7 +652,7 @@ void put_handle_map(struct shim_handle_map* map) {
if (map->fd_top == FD_NULL)
goto done;

for (int i = 0; i <= map->fd_top; i++) {
for (uint32_t i = 0; i <= map->fd_top; i++) {
if (!map->map[i])
continue;

Expand Down Expand Up @@ -672,7 +681,7 @@ int walk_handle_map(int (*callback)(struct shim_fd_handle*, struct shim_handle_m
if (map->fd_top == FD_NULL)
goto done;

for (int i = 0; i <= map->fd_top; i++) {
for (uint32_t i = 0; i <= map->fd_top; i++) {
if (!HANDLE_ALLOCATED(map->map[i]))
continue;

Expand Down Expand Up @@ -897,7 +906,7 @@ BEGIN_RS_FUNC(handle_map) {
lock(&handle_map->lock);

if (handle_map->fd_top != FD_NULL)
for (int i = 0; i <= handle_map->fd_top; i++) {
for (uint32_t i = 0; i <= handle_map->fd_top; i++) {
CP_REBASE(handle_map->map[i]);
if (HANDLE_ALLOCATED(handle_map->map[i])) {
CP_REBASE(handle_map->map[i]->handle);
Expand Down
6 changes: 3 additions & 3 deletions LibOS/shim/src/fs/proc/thread.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ int proc_thread_tid_list_names(struct shim_dentry* parent, readdir_callback_t ca
bool proc_thread_fd_name_exists(struct shim_dentry* parent, const char* name) {
__UNUSED(parent);
unsigned long fd;
if (pseudo_parse_ulong(name, FDTYPE_MAX, &fd) < 0)
if (pseudo_parse_ulong(name, UINT32_MAX, &fd) < 0)
return false;

struct shim_handle_map* handle_map = get_thread_handle_map(NULL);
Expand All @@ -253,7 +253,7 @@ int proc_thread_fd_list_names(struct shim_dentry* parent, readdir_callback_t cal
lock(&handle_map->lock);

int ret = 0;
for (int i = 0; i <= handle_map->fd_top; i++)
for (uint32_t i = 0; i <= handle_map->fd_top; i++)
if (handle_map->map[i] && handle_map->map[i]->handle) {
char name[11];
snprintf(name, sizeof(name), "%u", i);
Expand Down Expand Up @@ -288,7 +288,7 @@ static char* describe_handle(struct shim_handle* hdl) {

int proc_thread_fd_follow_link(struct shim_dentry* dent, char** out_target) {
unsigned long fd;
if (pseudo_parse_ulong(dent->name, FDTYPE_MAX, &fd) < 0)
if (pseudo_parse_ulong(dent->name, UINT32_MAX, &fd) < 0)
return -ENOENT;

struct shim_handle_map* handle_map = get_thread_handle_map(NULL);
Expand Down

0 comments on commit af787cd

Please sign in to comment.