From 4ae620f87bdaee4a0dca496a372809b85dc82d22 Mon Sep 17 00:00:00 2001 From: Gary Date: Wed, 26 Dec 2018 11:43:32 -0800 Subject: [PATCH] [PAL] Correct memory allocation in file_rename() and dir_rename() Use strdup() to correctly copy a string with NULL termination. Prevent memory leak by freeing {file,dir}.realpath when needed. --- Pal/src/host/FreeBSD/db_files.c | 43 +++++++++++++++++++++------ Pal/src/host/Linux-SGX/db_files.c | 48 +++++++++++++++++++++++-------- Pal/src/host/Linux/db_files.c | 42 +++++++++++++++++++++------ 3 files changed, 104 insertions(+), 29 deletions(-) diff --git a/Pal/src/host/FreeBSD/db_files.c b/Pal/src/host/FreeBSD/db_files.c index c6f3a577..e0825c81 100644 --- a/Pal/src/host/FreeBSD/db_files.c +++ b/Pal/src/host/FreeBSD/db_files.c @@ -130,10 +130,11 @@ static int file_close (PAL_HANDLE handle) int ret = INLINE_SYSCALL(close, 1, fd); + /* initial realpath is part of handle object and will be freed with it */ if (handle->file.realpath && - handle->file.realpath != (void *) handle + HANDLE_SIZE(file)) + handle->file.realpath != (void *) handle + HANDLE_SIZE(file)) { free((void *) handle->file.realpath); - + } return IS_ERR(ret) ? unix_to_pal_error(ERRNO(ret)) : 0; } @@ -270,12 +271,23 @@ static int file_attrsetbyhdl (PAL_HANDLE handle, static int file_rename (PAL_HANDLE handle, const char * type, const char * uri) { + char* tmp = strdup(uri); + if (!tmp) + return -PAL_ERROR_NOMEM; + int ret = INLINE_SYSCALL(rename, 2, handle->file.realpath, uri); - - if (IS_ERR(ret)) + if (IS_ERR(ret)) { + free(tmp); return unix_to_pal_error(ERRNO(ret)); + } - handle->file.realpath = malloc_copy(uri, strlen(uri)); + /* initial realpath is part of handle object and will be freed with it */ + if (handle->file.realpath && + handle->file.realpath != (void *) handle + HANDLE_SIZE(file)) { + free((void *) handle->file.realpath); + } + + handle->file.realpath = tmp; return 0; } @@ -459,9 +471,11 @@ static int dir_close (PAL_HANDLE handle) handle->dir.buf = handle->dir.ptr = handle->dir.end = NULL; } + /* initial realpath is part of handle object and will be freed with it */ if (handle->dir.realpath && - handle->dir.realpath != (void *) handle + HANDLE_SIZE(dir)) + handle->dir.realpath != (void *) handle + HANDLE_SIZE(dir)) { free((void *) handle->dir.realpath); + } if (IS_ERR(ret)) return -PAL_ERROR_BADHANDLE; @@ -489,12 +503,23 @@ static int dir_delete (PAL_HANDLE handle, int access) static int dir_rename (PAL_HANDLE handle, const char * type, const char * uri) { + char* tmp = strdup(uri); + if (!tmp) + return -PAL_ERROR_NOMEM; + int ret = INLINE_SYSCALL(rename, 2, handle->dir.realpath, uri); - - if (IS_ERR(ret)) + if (IS_ERR(ret)) { + free(tmp); return unix_to_pal_error(ERRNO(ret)); + } - handle->dir.realpath = malloc_copy(uri, strlen(uri)); + /* initial realpath is part of handle object and will be freed with it */ + if (handle->dir.realpath && + handle->dir.realpath != (void *) handle + HANDLE_SIZE(dir)) { + free((void *) handle->dir.realpath); + } + + handle->dir.realpath = tmp; return 0; } diff --git a/Pal/src/host/Linux-SGX/db_files.c b/Pal/src/host/Linux-SGX/db_files.c index 180863bf..ca3f5fea 100644 --- a/Pal/src/host/Linux-SGX/db_files.c +++ b/Pal/src/host/Linux-SGX/db_files.c @@ -165,6 +165,7 @@ static int file_close (PAL_HANDLE handle) int fd = handle->file.fd; ocall_close(fd); + /* initial realpath is part of handle object and will be freed with it */ if (handle->file.realpath && handle->file.realpath != (void *) handle + HANDLE_SIZE(file)) free((void *) handle->file.realpath); @@ -356,13 +357,24 @@ static int file_rename (PAL_HANDLE handle, const char * type, { if (!strcmp_static(type, "file")) return -PAL_ERROR_INVAL; - int ret = ocall_rename(handle->file.realpath, uri); - if (IS_ERR(ret)) - return unix_to_pal_error(ERRNO(ret)); - /* TODO: old realpath memory is potentially leaked here, and need - * to check for strdup memory allocation failure. */ - handle->file.realpath = strdup(uri); + char* tmp = strdup(uri); + if (!tmp) + return -PAL_ERROR_NOMEM; + + int ret = ocall_rename(handle->file.realpath, uri); + if (IS_ERR(ret)) { + free(tmp); + return unix_to_pal_error(ERRNO(ret)); + } + + /* initial realpath is part of handle object and will be freed with it */ + if (handle->file.realpath && + handle->file.realpath != (void *) handle + HANDLE_SIZE(file)) { + free((void *) handle->file.realpath); + } + + handle->file.realpath = tmp; return 0; } @@ -533,6 +545,7 @@ static int dir_close (PAL_HANDLE handle) handle->dir.buf = handle->dir.ptr = handle->dir.end = (PAL_PTR) NULL; } + /* initial realpath is part of handle object and will be freed with it */ if (handle->dir.realpath && handle->dir.realpath != (void *) handle + HANDLE_SIZE(dir)) free((void *) handle->dir.realpath); @@ -559,13 +572,24 @@ static int dir_rename (PAL_HANDLE handle, const char * type, { if (!strcmp_static(type, "dir")) return -PAL_ERROR_INVAL; - int ret = ocall_rename(handle->dir.realpath, uri); - if (IS_ERR(ret)) - return unix_to_pal_error(ERRNO(ret)); - /* TODO: old realpath memory is potentially leaked here, and need - * to check for strdup memory allocation failure. */ - handle->dir.realpath = strdup(uri); + char* tmp = strdup(uri); + if (!tmp) + return -PAL_ERROR_NOMEM; + + int ret = ocall_rename(handle->dir.realpath, uri); + if (IS_ERR(ret)) { + free(tmp); + return unix_to_pal_error(ERRNO(ret)); + } + + /* initial realpath is part of handle object and will be freed with it */ + if (handle->dir.realpath && + handle->dir.realpath != (void *) handle + HANDLE_SIZE(dir)) { + free((void *) handle->dir.realpath); + } + + handle->dir.realpath = tmp; return 0; } diff --git a/Pal/src/host/Linux/db_files.c b/Pal/src/host/Linux/db_files.c index 14d320a4..7add079a 100644 --- a/Pal/src/host/Linux/db_files.c +++ b/Pal/src/host/Linux/db_files.c @@ -133,9 +133,11 @@ static int file_close (PAL_HANDLE handle) int ret = INLINE_SYSCALL(close, 1, fd); + /* initial realpath is part of handle object and will be freed with it */ if (handle->file.realpath && - handle->file.realpath != (void *) handle + HANDLE_SIZE(file)) + handle->file.realpath != (void *) handle + HANDLE_SIZE(file)) { free((void *) handle->file.realpath); + } return IS_ERR(ret) ? unix_to_pal_error(ERRNO(ret)) : 0; } @@ -288,12 +290,23 @@ static int file_rename (PAL_HANDLE handle, const char * type, if (!strcmp_static(type, "file")) return -PAL_ERROR_INVAL; + char* tmp = strdup(uri); + if (!tmp) + return -PAL_ERROR_NOMEM; + int ret = INLINE_SYSCALL(rename, 2, handle->file.realpath, uri); - - if (IS_ERR(ret)) + if (IS_ERR(ret)) { + free(tmp); return unix_to_pal_error(ERRNO(ret)); + } - handle->file.realpath = malloc_copy(uri, strlen(uri)); + /* initial realpath is part of handle object and will be freed with it */ + if (handle->file.realpath && + handle->file.realpath != (void *) handle + HANDLE_SIZE(file)) { + free((void *) handle->file.realpath); + } + + handle->file.realpath = tmp; return 0; } @@ -484,9 +497,11 @@ static int dir_close (PAL_HANDLE handle) handle->dir.buf = handle->dir.ptr = handle->dir.end = (PAL_PTR) NULL; } + /* initial realpath is part of handle object and will be freed with it */ if (handle->dir.realpath && - handle->dir.realpath != (void *) handle + HANDLE_SIZE(dir)) + handle->dir.realpath != (void *) handle + HANDLE_SIZE(dir)) { free((void *) handle->dir.realpath); + } if (IS_ERR(ret)) return -PAL_ERROR_BADHANDLE; @@ -517,12 +532,23 @@ static int dir_rename (PAL_HANDLE handle, const char * type, if (!strcmp_static(type, "dir")) return -PAL_ERROR_INVAL; + char* tmp = strdup(uri); + if (!tmp) + return -PAL_ERROR_NOMEM; + int ret = INLINE_SYSCALL(rename, 2, handle->dir.realpath, uri); - - if (IS_ERR(ret)) + if (IS_ERR(ret)) { + free(tmp); return unix_to_pal_error(ERRNO(ret)); + } - handle->dir.realpath = malloc_copy(uri, strlen(uri)); + /* initial realpath is part of handle object and will be freed with it */ + if (handle->dir.realpath && + handle->dir.realpath != (void *) handle + HANDLE_SIZE(dir)) { + free((void *) handle->dir.realpath); + } + + handle->dir.realpath = tmp; return 0; }