From d06308df8bb098921a9eb519185410c400c3acb6 Mon Sep 17 00:00:00 2001 From: Siva Chandra Reddy Date: Thu, 22 Dec 2022 00:36:59 +0000 Subject: [PATCH] [libc] Handle allocation failures gracefully in FILE related API. Few uses of free have not yet been replaced by the custom operator delete yet. They will be done in a follow up patch. Reviewed By: lntue, michaelrj Differential Revision: https://reviews.llvm.org/D140526 --- libc/src/__support/File/CMakeLists.txt | 2 ++ libc/src/__support/File/file.cpp | 14 ++++++-- libc/src/__support/File/file.h | 35 ++++---------------- libc/src/__support/File/linux_file.cpp | 36 ++++++++++----------- libc/src/stdio/CMakeLists.txt | 1 + libc/src/stdio/fopencookie.cpp | 50 ++++++++++++++++++----------- libc/test/src/__support/File/CMakeLists.txt | 1 + libc/test/src/__support/File/file_test.cpp | 24 +++++--------- 8 files changed, 77 insertions(+), 86 deletions(-) diff --git a/libc/src/__support/File/CMakeLists.txt b/libc/src/__support/File/CMakeLists.txt index ef56a76..dd9bbee 100644 --- a/libc/src/__support/File/CMakeLists.txt +++ b/libc/src/__support/File/CMakeLists.txt @@ -12,6 +12,7 @@ add_object_library( file.h DEPENDS libc.include.errno + libc.src.__support.CPP.new libc.src.__support.CPP.span libc.src.__support.threads.mutex libc.src.__support.error_or @@ -40,6 +41,7 @@ if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS}_file.cpp) libc.include.fcntl libc.include.stdio libc.include.sys_syscall + libc.src.__support.CPP.new libc.src.__support.OSUtil.osutil libc.src.errno.errno libc.src.__support.error_or diff --git a/libc/src/__support/File/file.cpp b/libc/src/__support/File/file.cpp index 1b6d2cb..1493633 100644 --- a/libc/src/__support/File/file.cpp +++ b/libc/src/__support/File/file.cpp @@ -8,6 +8,7 @@ #include "file.h" +#include "src/__support/CPP/new.h" #include "src/__support/CPP/span.h" #include // For error macros @@ -377,16 +378,23 @@ int File::set_buffer(void *buffer, size_t size, int buffer_mode) { // We exclude the case of buffer_mode == _IONBF in this branch // because we don't need to allocate buffer in such a case. if (own_buf) { - buf = realloc(buf, size); + // This is one of the places where use a C allocation functon + // as C++ does not have an equivalent of realloc. + buf = reinterpret_cast(realloc(buf, size)); + if (buf == nullptr) + return ENOMEM; } else { - buf = malloc(size); + AllocChecker ac; + buf = new (ac) uint8_t[size]; + if (!ac) + return ENOMEM; own_buf = true; } bufsize = size; // TODO: Handle allocation failures. } else { if (own_buf) - free(buf); + delete buf; if (buffer_mode != _IONBF) { buf = static_cast(buffer); bufsize = size; diff --git a/libc/src/__support/File/file.h b/libc/src/__support/File/file.h index 7ba6c27..3828694 100644 --- a/libc/src/__support/File/file.h +++ b/libc/src/__support/File/file.h @@ -89,9 +89,9 @@ private: // For files which are readable, we should be able to support one ungetc // operation even if |buf| is nullptr. So, in the constructor of File, we // set |buf| to point to this buffer character. - char ungetc_buf; + uint8_t ungetc_buf; - void *buf; // Pointer to the stream buffer for buffered streams + uint8_t *buf; // Pointer to the stream buffer for buffered streams size_t bufsize; // Size of the buffer pointed to by |buf|. // Buffering mode to used to buffer. @@ -151,7 +151,7 @@ public: // is zero. This way, we will not have to employ the semantics of // the set_buffer method and allocate a buffer. constexpr File(WriteFunc *wf, ReadFunc *rf, SeekFunc *sf, CloseFunc *cf, - FlushFunc *ff, void *buffer, size_t buffer_size, + FlushFunc *ff, uint8_t *buffer, size_t buffer_size, int buffer_mode, bool owned, ModeFlags modeflags) : platform_write(wf), platform_read(rf), platform_seek(sf), platform_close(cf), platform_flush(ff), mutex(false, false, false), @@ -161,31 +161,6 @@ public: adjust_buf(); } - // This function helps initialize the various fields of the File data - // structure after a allocating memory for it via a call to malloc. - static void init(File *f, WriteFunc *wf, ReadFunc *rf, SeekFunc *sf, - CloseFunc *cf, FlushFunc *ff, void *buffer, - size_t buffer_size, int buffer_mode, bool owned, - ModeFlags modeflags) { - Mutex::init(&f->mutex, false, false, false); - f->platform_write = wf; - f->platform_read = rf; - f->platform_seek = sf; - f->platform_close = cf; - f->platform_flush = ff; - f->buf = reinterpret_cast(buffer); - f->bufsize = buffer_size; - f->bufmode = buffer_mode; - f->own_buf = owned; - f->mode = modeflags; - - f->prev_op = FileOp::NONE; - f->read_limit = f->pos = 0; - f->eof = f->err = false; - - f->adjust_buf(); - } - // Buffered write of |len| bytes from |data| without the file lock. FileIOResult write_unlocked(const void *data, size_t len); @@ -234,7 +209,9 @@ public: // if: // 1. |buffer| is not a nullptr but |size| is zero. // 2. |buffer_mode| is not one of _IOLBF, IOFBF or _IONBF. - // In both the above cases, error returned in EINVAL. + // 3. If an allocation was required but the allocation failed. + // For cases 1 and 2, the error returned in EINVAL. For case 3, error returned + // is ENOMEM. int set_buffer(void *buffer, size_t size, int buffer_mode); // Closes the file stream and frees up all resources owned by it. diff --git a/libc/src/__support/File/linux_file.cpp b/libc/src/__support/File/linux_file.cpp index b7ff089..13de7c4 100644 --- a/libc/src/__support/File/linux_file.cpp +++ b/libc/src/__support/File/linux_file.cpp @@ -8,12 +8,12 @@ #include "file.h" +#include "src/__support/CPP/new.h" #include "src/__support/OSUtil/syscall.h" // For internal syscall function. #include // For error macros #include // For mode_t and other flags to the open syscall #include -#include // For malloc #include // For syscall numbers namespace __llvm_libc { @@ -32,20 +32,12 @@ class LinuxFile : public File { int fd; public: - constexpr LinuxFile(int file_descriptor, void *buffer, size_t buffer_size, + constexpr LinuxFile(int file_descriptor, uint8_t *buffer, size_t buffer_size, int buffer_mode, bool owned, File::ModeFlags modeflags) : File(&write_func, &read_func, &seek_func, &close_func, flush_func, buffer, buffer_size, buffer_mode, owned, modeflags), fd(file_descriptor) {} - static void init(LinuxFile *f, int file_descriptor, void *buffer, - size_t buffer_size, int buffer_mode, bool owned, - File::ModeFlags modeflags) { - File::init(f, &write_func, &read_func, &seek_func, &close_func, &flush_func, - buffer, buffer_size, buffer_mode, owned, modeflags); - f->fd = file_descriptor; - } - int get_fd() const { return fd; } }; @@ -149,15 +141,21 @@ ErrorOr openfile(const char *path, const char *mode) { #error "SYS_open and SYS_openat syscalls not available to perform a file open." #endif - if (fd < 0) { + if (fd < 0) return Error(-fd); - // return {nullptr, -fd}; - } - void *buffer = malloc(File::DEFAULT_BUFFER_SIZE); - auto *file = reinterpret_cast(malloc(sizeof(LinuxFile))); - LinuxFile::init(file, fd, buffer, File::DEFAULT_BUFFER_SIZE, _IOFBF, true, - modeflags); + uint8_t *buffer; + { + AllocChecker ac; + buffer = new (ac) uint8_t[File::DEFAULT_BUFFER_SIZE]; + if (!ac) + return Error(ENOMEM); + } + AllocChecker ac; + auto *file = new (ac) + LinuxFile(fd, buffer, File::DEFAULT_BUFFER_SIZE, _IOFBF, true, modeflags); + if (!ac) + return Error(ENOMEM); return file; } @@ -167,13 +165,13 @@ int get_fileno(File *f) { } constexpr size_t STDIN_BUFFER_SIZE = 512; -char stdin_buffer[STDIN_BUFFER_SIZE]; +uint8_t stdin_buffer[STDIN_BUFFER_SIZE]; static LinuxFile StdIn(0, stdin_buffer, STDIN_BUFFER_SIZE, _IOFBF, false, File::ModeFlags(File::OpenMode::READ)); File *stdin = &StdIn; constexpr size_t STDOUT_BUFFER_SIZE = 1024; -char stdout_buffer[STDOUT_BUFFER_SIZE]; +uint8_t stdout_buffer[STDOUT_BUFFER_SIZE]; static LinuxFile StdOut(1, stdout_buffer, STDOUT_BUFFER_SIZE, _IOLBF, false, File::ModeFlags(File::OpenMode::APPEND)); File *stdout = &StdOut; diff --git a/libc/src/stdio/CMakeLists.txt b/libc/src/stdio/CMakeLists.txt index d10f527..bb765ee 100644 --- a/libc/src/stdio/CMakeLists.txt +++ b/libc/src/stdio/CMakeLists.txt @@ -355,6 +355,7 @@ add_entrypoint_object( fopencookie.h DEPENDS libc.include.stdio + libc.src.__support.CPP.new libc.src.__support.File.file ) diff --git a/libc/src/stdio/fopencookie.cpp b/libc/src/stdio/fopencookie.cpp index bd98460..a48e28f1 100644 --- a/libc/src/stdio/fopencookie.cpp +++ b/libc/src/stdio/fopencookie.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "src/stdio/fopencookie.h" +#include "src/__support/CPP/new.h" #include "src/__support/File/file.h" #include @@ -18,12 +19,25 @@ namespace __llvm_libc { namespace { class CookieFile : public __llvm_libc::File { -public: void *cookie; cookie_io_functions_t ops; + + static FileIOResult cookie_write(File *f, const void *data, size_t size); + static FileIOResult cookie_read(File *f, void *data, size_t size); + static ErrorOr cookie_seek(File *f, long offset, int whence); + static int cookie_close(File *f); + static int cookie_flush(File *); + +public: + CookieFile(void *c, cookie_io_functions_t cops, uint8_t *buffer, + size_t bufsize, File::ModeFlags mode) + : File(&cookie_write, &cookie_read, &CookieFile::cookie_seek, + &cookie_close, &cookie_flush, buffer, bufsize, + 0 /* default buffering mode */, true /* File owns buffer */, mode), + cookie(c), ops(cops) {} }; -FileIOResult write_func(File *f, const void *data, size_t size) { +FileIOResult CookieFile::cookie_write(File *f, const void *data, size_t size) { auto cookie_file = reinterpret_cast(f); if (cookie_file->ops.write == nullptr) return 0; @@ -31,7 +45,7 @@ FileIOResult write_func(File *f, const void *data, size_t size) { reinterpret_cast(data), size); } -FileIOResult read_func(File *f, void *data, size_t size) { +FileIOResult CookieFile::cookie_read(File *f, void *data, size_t size) { auto cookie_file = reinterpret_cast(f); if (cookie_file->ops.read == nullptr) return 0; @@ -39,7 +53,7 @@ FileIOResult read_func(File *f, void *data, size_t size) { reinterpret_cast(data), size); } -ErrorOr seek_func(File *f, long offset, int whence) { +ErrorOr CookieFile::cookie_seek(File *f, long offset, int whence) { auto cookie_file = reinterpret_cast(f); if (cookie_file->ops.seek == nullptr) { return Error(EINVAL); @@ -52,34 +66,32 @@ ErrorOr seek_func(File *f, long offset, int whence) { return -1; } -int close_func(File *f) { +int CookieFile::cookie_close(File *f) { auto cookie_file = reinterpret_cast(f); if (cookie_file->ops.close == nullptr) return 0; return cookie_file->ops.close(cookie_file->cookie); } -int flush_func(File *) { return 0; } +int CookieFile::cookie_flush(File *) { return 0; } } // anonymous namespace LLVM_LIBC_FUNCTION(::FILE *, fopencookie, (void *cookie, const char *mode, cookie_io_functions_t ops)) { - auto modeflags = File::mode_flags(mode); - void *buffer = malloc(File::DEFAULT_BUFFER_SIZE); - auto *file = reinterpret_cast(malloc(sizeof(CookieFile))); - if (file == nullptr) + uint8_t *buffer; + { + AllocChecker ac; + buffer = new (ac) uint8_t[File::DEFAULT_BUFFER_SIZE]; + if (!ac) + return nullptr; + } + AllocChecker ac; + auto *file = new (ac) CookieFile( + cookie, ops, buffer, File::DEFAULT_BUFFER_SIZE, File::mode_flags(mode)); + if (!ac) return nullptr; - - File::init(file, &write_func, &read_func, &seek_func, &close_func, - &flush_func, buffer, File::DEFAULT_BUFFER_SIZE, - 0, // Default buffering style - true, // Owned buffer - modeflags); - file->cookie = cookie; - file->ops = ops; - return reinterpret_cast<::FILE *>(file); } diff --git a/libc/test/src/__support/File/CMakeLists.txt b/libc/test/src/__support/File/CMakeLists.txt index 17ca25b..4ee7a32 100644 --- a/libc/test/src/__support/File/CMakeLists.txt +++ b/libc/test/src/__support/File/CMakeLists.txt @@ -16,6 +16,7 @@ add_libc_unittest( libc.include.errno libc.include.stdio libc.include.stdlib + libc.src.__support.CPP.new libc.src.__support.File.file ) diff --git a/libc/test/src/__support/File/file_test.cpp b/libc/test/src/__support/File/file_test.cpp index 2931fea..f3690e4 100644 --- a/libc/test/src/__support/File/file_test.cpp +++ b/libc/test/src/__support/File/file_test.cpp @@ -6,6 +6,7 @@ // //===----------------------------------------------------------------------===// +#include "src/__support/CPP/new.h" #include "src/__support/File/file.h" #include "src/__support/error_or.h" #include "utils/UnitTest/MemoryMatcher.h" @@ -37,24 +38,13 @@ public: explicit StringFile(char *buffer, size_t buflen, int bufmode, bool owned, ModeFlags modeflags) : __llvm_libc::File(&str_write, &str_read, &str_seek, &str_close, - &str_flush, buffer, buflen, bufmode, owned, - modeflags), + &str_flush, reinterpret_cast(buffer), + buflen, bufmode, owned, modeflags), pos(0), eof_marker(0), write_append(false) { if (modeflags & static_cast(__llvm_libc::File::OpenMode::APPEND)) write_append = true; } - void init(char *buffer, size_t buflen, int bufmode, bool owned, - ModeFlags modeflags) { - File::init(this, &str_write, &str_read, &str_seek, &str_close, &str_flush, - buffer, buflen, bufmode, owned, modeflags); - pos = eof_marker = 0; - if (modeflags & static_cast(__llvm_libc::File::OpenMode::APPEND)) - write_append = true; - else - write_append = false; - } - void reset() { pos = 0; } size_t get_pos() const { return pos; } char *get_str() { return str; } @@ -112,9 +102,11 @@ ErrorOr StringFile::str_seek(__llvm_libc::File *f, long offset, StringFile *new_string_file(char *buffer, size_t buflen, int bufmode, bool owned, const char *mode) { - StringFile *f = reinterpret_cast(malloc(sizeof(StringFile))); - f->init(buffer, buflen, bufmode, owned, __llvm_libc::File::mode_flags(mode)); - return f; + __llvm_libc::AllocChecker ac; + // We will just assume the allocation succeeds. We cannot test anything + // otherwise. + return new (ac) StringFile(buffer, buflen, bufmode, owned, + __llvm_libc::File::mode_flags(mode)); } TEST(LlvmLibcFileTest, WriteOnly) { -- 2.7.4