[libc-commits] [libc] d06308d - [libc] Handle allocation failures gracefully in FILE related API.
Siva Chandra Reddy via libc-commits
libc-commits at lists.llvm.org
Thu Dec 22 11:30:24 PST 2022
Author: Siva Chandra Reddy
Date: 2022-12-22T19:30:16Z
New Revision: d06308df8bb098921a9eb519185410c400c3acb6
URL: https://github.com/llvm/llvm-project/commit/d06308df8bb098921a9eb519185410c400c3acb6
DIFF: https://github.com/llvm/llvm-project/commit/d06308df8bb098921a9eb519185410c400c3acb6.diff
LOG: [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
Added:
Modified:
libc/src/__support/File/CMakeLists.txt
libc/src/__support/File/file.cpp
libc/src/__support/File/file.h
libc/src/__support/File/linux_file.cpp
libc/src/stdio/CMakeLists.txt
libc/src/stdio/fopencookie.cpp
libc/test/src/__support/File/CMakeLists.txt
libc/test/src/__support/File/file_test.cpp
Removed:
################################################################################
diff --git a/libc/src/__support/File/CMakeLists.txt b/libc/src/__support/File/CMakeLists.txt
index ef56a767135bd..dd9bbeea79982 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 1b6d2cb859892..1493633718e6d 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 <errno.h> // 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<uint8_t *>(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<uint8_t *>(buffer);
bufsize = size;
diff --git a/libc/src/__support/File/file.h b/libc/src/__support/File/file.h
index 7ba6c27f2d4e1..3828694cbf032 100644
--- a/libc/src/__support/File/file.h
+++ b/libc/src/__support/File/file.h
@@ -89,9 +89,9 @@ class File {
// 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 @@ class File {
// 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 @@ class File {
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<uint8_t *>(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 @@ class File {
// 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 b7ff089905ff8..13de7c46994e2 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 <errno.h> // For error macros
#include <fcntl.h> // For mode_t and other flags to the open syscall
#include <stdio.h>
-#include <stdlib.h> // For malloc
#include <sys/syscall.h> // 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<File *> 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<LinuxFile *>(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 d10f527627ba4..bb765eeb5af29 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 bd9846020f746..a48e28f1324e9 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 <errno.h>
@@ -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<long> 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<CookieFile *>(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<const char *>(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<CookieFile *>(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<char *>(data), size);
}
-ErrorOr<long> seek_func(File *f, long offset, int whence) {
+ErrorOr<long> CookieFile::cookie_seek(File *f, long offset, int whence) {
auto cookie_file = reinterpret_cast<CookieFile *>(f);
if (cookie_file->ops.seek == nullptr) {
return Error(EINVAL);
@@ -52,34 +66,32 @@ ErrorOr<long> 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<CookieFile *>(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<CookieFile *>(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 17ca25b7055d8..4ee7a3255aee5 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 2931fea9e57c7..f3690e492534e 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 @@ class StringFile : public __llvm_libc::File {
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<uint8_t *>(buffer),
+ buflen, bufmode, owned, modeflags),
pos(0), eof_marker(0), write_append(false) {
if (modeflags & static_cast<ModeFlags>(__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<ModeFlags>(__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<long> 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<StringFile *>(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) {
More information about the libc-commits
mailing list