From efff53149dd904257cc65e7be91199f7b179164c Mon Sep 17 00:00:00 2001 From: Andrei Medar Date: Wed, 7 Oct 2020 10:07:13 +0000 Subject: [PATCH] Implemented requested changes (variable names, functions return absl::Status/absl::StatusOr) --- oss-internship-2020/libarchive/CMakeLists.txt | 5 +- oss-internship-2020/libarchive/README.md | 19 +- .../libarchive/examples/sandbox.h | 11 +- .../libarchive/examples/sapi_minitar.cc | 497 +++++++++++------- .../libarchive/examples/sapi_minitar.h | 23 +- .../libarchive/examples/sapi_minitar_main.cc | 70 ++- .../libarchive/test/CMakeLists.txt | 2 +- .../libarchive/test/minitar_test.cc | 72 ++- 8 files changed, 431 insertions(+), 268 deletions(-) diff --git a/oss-internship-2020/libarchive/CMakeLists.txt b/oss-internship-2020/libarchive/CMakeLists.txt index 3b8e375..9f4aa60 100644 --- a/oss-internship-2020/libarchive/CMakeLists.txt +++ b/oss-internship-2020/libarchive/CMakeLists.txt @@ -20,7 +20,8 @@ set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED 17) # Build SAPI library -set(SAPI_ROOT "" CACHE PATH "Path to the Sandboxed API source tree") +#set(SAPI_ROOT "" CACHE PATH "Path to the Sandboxed API source tree") +set(SAPI_ROOT "/usr/local/google/home/amedar/internship/sandboxed-api" CACHE PATH "Path to the Sandboxed API source tree") include(FetchContent) FetchContent_Declare( @@ -28,7 +29,7 @@ FetchContent_Declare( GIT_REPOSITORY https://github.com/libarchive/libarchive PATCH_COMMAND cd libarchive && patch < ${CMAKE_SOURCE_DIR}/patches/header.patch && patch < ${CMAKE_SOURCE_DIR}/patches/archive_virtual.patch ) - FetchContent_MakeAvailable(libarchive) +FetchContent_MakeAvailable(libarchive) add_subdirectory("${SAPI_ROOT}" "${CMAKE_BINARY_DIR}/sandboxed-api-build" diff --git a/oss-internship-2020/libarchive/README.md b/oss-internship-2020/libarchive/README.md index 700e635..b41c50a 100644 --- a/oss-internship-2020/libarchive/README.md +++ b/oss-internship-2020/libarchive/README.md @@ -4,27 +4,24 @@ Sandboxed version of the [libarchive](https://www.libarchive.org/) minitar [exam ## Build - - -`mkdir -p build && cd build` - -`cmake .. -G Ninja` - -`cmake --build .` +``` +mkdir -p build && cd build +cmake .. -G Ninja +cmake --build . +``` The example binary file can be found at **build/examples/sapi_minitar** and the unit tests at **build/test/sapi_minitar_test**. ## Patches -The original libarchive code required patching since one of the custom types produced errors with libclang Python errors. The patches are applied automatically during the build step and they do not modify the functionality of the library. The repository is also fetched automatically. +The original libarchive code required patching since one of the custom types produced errors with libclang Python byndings. The patches are applied automatically during the build step and they do not modify the functionality of the library. The repository is also fetched automatically. ## Examples In this project, the minitar example is sandboxed. The code is found in the **examples** directory and is structured as follows: - **sapi_minitar_main.cc** - ***main*** function of the minitar tool. This is mostly similar to the original example. -- **sapi_minitar.h** and **sapi_minitar.cc** - The two main functions (***create*** and ***extract***) and also other helper functions. +- **sapi_minitar.h** and **sapi_minitar.cc** - The two main functions (***CreateArchive*** and ***ExtractArchive***) and other helper functions. - **sandbox.h** - Custom security policies, depending on the whether the user creates or extracts an archive. On top of that, unit tests can be found in the **test/minitar_test.cc** file. @@ -47,4 +44,4 @@ The available options are: - *Z* - Default compression. - *z* - Compress with GZIP. -If no compression method is chosen (in the case of archive creation) the files will only be archived. +If no compression method is chosen (in the case of archive creation) the files will only be stored. diff --git a/oss-internship-2020/libarchive/examples/sandbox.h b/oss-internship-2020/libarchive/examples/sandbox.h index 8357b38..a40a306 100644 --- a/oss-internship-2020/libarchive/examples/sandbox.h +++ b/oss-internship-2020/libarchive/examples/sandbox.h @@ -18,7 +18,7 @@ #include #include -#include "libarchive_sapi.sapi.h" +#include "libarchive_sapi.sapi.h" // NOLINT(build/include) #include "sandboxed_api/sandbox2/util/bpf_helper.h" #include "sandboxed_api/sandbox2/util/fileops.h" @@ -28,8 +28,8 @@ // create the file without having access to anything else. class SapiLibarchiveSandboxCreate : public LibarchiveSandbox { public: - explicit SapiLibarchiveSandboxCreate(const std::vector& files, - absl::string_view archive_path) + SapiLibarchiveSandboxCreate(const std::vector& files, + absl::string_view archive_path) : files_(files), archive_path_(archive_path) {} private: @@ -92,9 +92,8 @@ class SapiLibarchiveSandboxCreate : public LibarchiveSandbox { // pe placed correctly without offering additional permission. class SapiLibarchiveSandboxExtract : public LibarchiveSandbox { public: - explicit SapiLibarchiveSandboxExtract(absl::string_view archive_path, - const int do_extract, - absl::string_view tmp_dir) + SapiLibarchiveSandboxExtract(absl::string_view archive_path, int do_extract, + absl::string_view tmp_dir) : archive_path_(archive_path), do_extract_(do_extract), tmp_dir_(tmp_dir) {} diff --git a/oss-internship-2020/libarchive/examples/sapi_minitar.cc b/oss-internship-2020/libarchive/examples/sapi_minitar.cc index 2225cc9..0af460e 100644 --- a/oss-internship-2020/libarchive/examples/sapi_minitar.cc +++ b/oss-internship-2020/libarchive/examples/sapi_minitar.cc @@ -12,12 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "sapi_minitar.h" +#include "sapi_minitar.h" // NOLINT(build/include) +#include "absl/status/status.h" #include "sandboxed_api/sandbox2/util/path.h" +#include "sandboxed_api/util/status_macros.h" -void create(const char* initial_filename, int compress, const char** argv, - bool verbose) { +absl::Status CreateArchive(const char* initial_filename, int compress, + const char** argv, bool verbose) { // We split the filename path into dirname and filename. To the filename we // prepend "/output/"" so that it will work with the security policy. std::string abs_path = MakeAbsolutePathAtCWD(std::string(initial_filename)); @@ -42,91 +44,133 @@ void create(const char* initial_filename, int compress, const char** argv, // Initialize sandbox and api objects. SapiLibarchiveSandboxCreate sandbox(absolute_paths, archive_path); - CHECK(sandbox.Init().ok()) << "Error during sandbox initialization"; + SAPI_RETURN_IF_ERROR(sandbox.Init()); LibarchiveApi api(&sandbox); - archive* ret = api.archive_write_new().value(); - CHECK(ret != NULL) << "Failed to create write archive"; + SAPI_ASSIGN_OR_RETURN(archive * ret_archive, api.archive_write_new()); + if (ret_archive == nullptr) { + return absl::FailedPreconditionError("Failed to create write archive"); + } // Treat the pointer as remote. There is no need to copy the data // to the client process. - sapi::v::RemotePtr a(ret); + sapi::v::RemotePtr a(ret_archive); - int ret2; + int rc; + std::string msg; - switch (compress) { - case 'j': - case 'y': - CHECK(api.archive_write_add_filter_bzip2(&a).value() == ARCHIVE_OK) - << "Unexpected result from write_add_filter_bzip2 call"; - break; - case 'Z': - CHECK(api.archive_write_add_filter_compress(&a).value() == ARCHIVE_OK) - << "Unexpected result from write_add_filter_compress call"; - break; - case 'z': - CHECK(api.archive_write_add_filter_gzip(&a).value() == ARCHIVE_OK) - << "Unexpected result from write_add_filter_gzip call"; - break; - default: - CHECK(api.archive_write_add_filter_none(&a).value() == ARCHIVE_OK) - << "Unexpected result from write_add_filter_none call"; - break; + // switch (compress) { + // case 'j': + // case 'y': + if (compress == 'j' || compress == 'y') { + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_add_filter_bzip2(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_add_filter_bzip2 call"); + } + // break; + } else if (compress == 'Z') { + // case 'Z': + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_add_filter_compress(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_add_filter_compress call"); + } + // break; + } else if (compress == 'z') { + // case 'z': + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_add_filter_gzip(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_add_filter_gzip call"); + } + // break; + } else { + // default: + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_add_filter_none(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_add_filter_none call"); + } + // break; } - CHECK(api.archive_write_set_format_ustar(&a).value() == ARCHIVE_OK) - << "Unexpected result from write_set_format_ustar call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_set_format_ustar(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_set_format_ustar call"); + } const char* filename_ptr = filename.data(); - if (filename_ptr != NULL && strcmp(filename_ptr, "-") == 0) { - filename_ptr = NULL; + if (filename_ptr != nullptr && strcmp(filename_ptr, "-") == 0) { + filename_ptr = nullptr; } - CHECK(api.archive_write_open_filename( - &a, sapi::v::ConstCStr(filename_ptr).PtrBefore()) - .value() == ARCHIVE_OK) - << "Unexpected result from write_open_filename call"; + SAPI_ASSIGN_OR_RETURN(rc, + api.archive_write_open_filename( + &a, sapi::v::ConstCStr(filename_ptr).PtrBefore())); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_open_filename call"); + } int file_idx = 0; // We can directly use the vectors defined before. for (int file_idx = 0; file_idx < absolute_paths.size(); ++file_idx) { - ret = api.archive_read_disk_new().value(); - CHECK(ret != NULL) << "Failed to create read_disk archive"; + SAPI_ASSIGN_OR_RETURN(ret_archive, api.archive_read_disk_new()); + if (ret_archive == nullptr) { + return absl::FailedPreconditionError( + "Failed to create read_disk archive"); + } - sapi::v::RemotePtr disk(ret); + sapi::v::RemotePtr disk(ret_archive); - CHECK(api.archive_read_disk_set_standard_lookup(&disk).value() == - ARCHIVE_OK) - << "Unexpected result from read_disk_set_standard_lookup call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_disk_set_standard_lookup(&disk)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from read_disk_set_standard_lookup call"); + } // We use the absolute path first. - CHECK( + SAPI_ASSIGN_OR_RETURN( + rc, api.archive_read_disk_open( - &disk, - sapi::v::ConstCStr(absolute_paths[file_idx].c_str()).PtrBefore()) - .value() == ARCHIVE_OK) - << CheckStatusAndGetString(api.archive_error_string(&disk), sandbox); + &disk, + sapi::v::ConstCStr(absolute_paths[file_idx].c_str()).PtrBefore())); + if (rc != ARCHIVE_OK) { + SAPI_ASSIGN_OR_RETURN(msg, CheckStatusAndGetString( + api.archive_error_string(&disk), sandbox)); + return absl::FailedPreconditionError(msg); + } - for (;;) { - archive_entry* ret3; - ret3 = api.archive_entry_new().value(); + while (true) { + archive_entry* ret_archive_entry; + SAPI_ASSIGN_OR_RETURN(ret_archive_entry, api.archive_entry_new()); - CHECK(ret3 != NULL) << "Failed to create archive_entry"; + if (ret_archive_entry == nullptr) { + return absl::FailedPreconditionError("Failed to create archive_entry"); + } - sapi::v::RemotePtr entry(ret3); + sapi::v::RemotePtr entry(ret_archive_entry); - ret2 = api.archive_read_next_header2(&disk, &entry).value(); + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_next_header2(&disk, &entry)); - if (ret2 == ARCHIVE_EOF) { + if (rc == ARCHIVE_EOF) { break; } - CHECK(ret2 == ARCHIVE_OK) - << CheckStatusAndGetString(api.archive_error_string(&disk), sandbox); + if (rc != ARCHIVE_OK) { + SAPI_ASSIGN_OR_RETURN( + msg, + CheckStatusAndGetString(api.archive_error_string(&disk), sandbox)); + return absl::FailedPreconditionError(msg); + } - CHECK(api.archive_read_disk_descend(&disk).ok()) - << "read_disk_descend call failed"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_disk_descend(&disk)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError("read_disk_descend call failed"); + } // After using the absolute path before, we now need to add the pathname // to the archive entry. This would help store the files by their relative @@ -139,8 +183,9 @@ void create(const char* initial_filename, int compress, const char** argv, // "/absolute/path/test_files" and the files inside of it will become // similar to "/absolute/path/test_files/file1" // which we then change to "test_files/file1" so that it is relative. - std::string path_name = - CheckStatusAndGetString(api.archive_entry_pathname(&entry), sandbox); + SAPI_ASSIGN_OR_RETURN( + std::string path_name, + CheckStatusAndGetString(api.archive_entry_pathname(&entry), sandbox)); path_name.replace(path_name.begin(), path_name.begin() + absolute_paths[file_idx].length(), @@ -153,41 +198,48 @@ void create(const char* initial_filename, int compress, const char** argv, path_name.erase(path_name.begin(), path_name.begin() + found); } - found = path_name.rfind("../"); + // Search either for the last '/../' or check if + // the path has '../' in the beginning. + found = path_name.rfind("/../"); if (found != std::string::npos) { - path_name = path_name.substr(found + 3); + path_name = path_name.substr(found + 4); + } else if (path_name.substr(0, 3) == "../") { + path_name = path_name.substr(3); } - CHECK(api.archive_entry_set_pathname( - &entry, sapi::v::ConstCStr(path_name.c_str()).PtrBefore()) - .ok()) - << "Could not set pathname"; + SAPI_RETURN_IF_ERROR(api.archive_entry_set_pathname( + &entry, sapi::v::ConstCStr(path_name.c_str()).PtrBefore())); if (verbose) { - std::cout << CheckStatusAndGetString(api.archive_entry_pathname(&entry), - sandbox) - << std::endl; + SAPI_ASSIGN_OR_RETURN( + msg, CheckStatusAndGetString(api.archive_entry_pathname(&entry), + sandbox)); + std::cout << msg << std::endl; } - ret2 = api.archive_write_header(&a, &entry).value(); + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_header(&a, &entry)); - if (ret2 < ARCHIVE_OK) { - std::cout << CheckStatusAndGetString(api.archive_error_string(&a), - sandbox) - << std::endl; + if (rc < ARCHIVE_OK) { + SAPI_ASSIGN_OR_RETURN(msg, CheckStatusAndGetString( + api.archive_error_string(&a), sandbox)); + std::cout << msg << std::endl; + } + if (rc == ARCHIVE_FATAL) { + return absl::FailedPreconditionError( + "Unexpected result from write_header call"); } - CHECK(ret2 != ARCHIVE_FATAL) - << "Unexpected result from write_header call"; // In the following section, the calls (read, archive_write_data) are done // on the sandboxed process since we do not need to transfer the data in // the client process. - if (ret2 > ARCHIVE_FAILED) { - int fd = open(CheckStatusAndGetString( - api.archive_entry_sourcepath(&entry), sandbox) - .c_str(), - O_RDONLY); - CHECK(fd >= 0) << "Could not open file"; + if (rc > ARCHIVE_FAILED) { + SAPI_ASSIGN_OR_RETURN( + msg, CheckStatusAndGetString(api.archive_entry_sourcepath(&entry), + sandbox)); + int fd = open(msg.c_str(), O_RDONLY); + if (fd < 0) { + return absl::FailedPreconditionError("Could not open file"); + } sapi::v::Fd sapi_fd(fd); sapi::v::Int read_ret; @@ -196,51 +248,63 @@ void create(const char* initial_filename, int compress, const char** argv, // We allocate the buffer remotely and then we can simply use the // remote pointer(with PtrNone). - CHECK(sandbox.Allocate(&buff, true).ok()) - << "Could not allocate remote buffer"; + // This allows us to keep the data in the remote process without always + // transferring the memory. + SAPI_RETURN_IF_ERROR(sandbox.Allocate(&buff, true)); // We can use sapi methods that help us with file descriptors. - CHECK(sandbox.TransferToSandboxee(&sapi_fd).ok()) - << "Could not transfer file descriptor"; + SAPI_RETURN_IF_ERROR(sandbox.TransferToSandboxee(&sapi_fd)); - CHECK(sandbox.Call("read", &read_ret, &sapi_fd, buff.PtrNone(), &ssize) - .ok()) - << "Read call failed"; + SAPI_RETURN_IF_ERROR( + sandbox.Call("read", &read_ret, &sapi_fd, buff.PtrNone(), &ssize)); while (read_ret.GetValue() > 0) { - CHECK(api.archive_write_data(&a, buff.PtrNone(), read_ret.GetValue()) - .ok()) - << "write_data call failed"; + SAPI_ASSIGN_OR_RETURN( + rc, + api.archive_write_data(&a, buff.PtrNone(), read_ret.GetValue())); - CHECK( - sandbox.Call("read", &read_ret, &sapi_fd, buff.PtrNone(), &ssize) - .ok()) - << "Read call failed"; + SAPI_RETURN_IF_ERROR(sandbox.Call("read", &read_ret, &sapi_fd, + buff.PtrNone(), &ssize)); } // sapi_fd variable goes out of scope here so both the local and the // remote file descriptors are closed. } - CHECK(api.archive_entry_free(&entry).ok()) << "entry_free call failed"; + SAPI_RETURN_IF_ERROR(api.archive_entry_free(&entry)); } - CHECK(api.archive_read_close(&disk).value() == ARCHIVE_OK) - << "Unexpected result from read_close call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_close(&disk)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from read_close call"); + } - CHECK(api.archive_read_free(&disk).value() == ARCHIVE_OK) - << "Unexpected result from read_free call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_free(&disk)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from read_free call"); + } } - CHECK(api.archive_write_close(&a).value() == ARCHIVE_OK) - << "Unexpected result from write_close call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_close(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_close call"); + } - CHECK(api.archive_write_free(&a).value() == ARCHIVE_OK) - << "Unexpected result from write_free call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_free(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_free call"); + } + + return absl::OkStatus(); } -void extract(const char* filename, int do_extract, int flags, bool verbose) { +absl::Status ExtractArchive(const char* filename, int do_extract, int flags, + bool verbose) { std::string tmp_dir; if (do_extract) { - tmp_dir = CreateTempDirAtCWD(); + SAPI_ASSIGN_OR_RETURN(tmp_dir, CreateTempDirAtCWD()); } // We can use a struct like this in order to delete the temporary @@ -257,73 +321,107 @@ void extract(const char* filename, int do_extract, int flags, bool verbose) { // We should only delete it if the do_extract flag is true which // means that this struct is instantiated only in that case. - std::unique_ptr cleanup_ptr; - if (do_extract) { - cleanup_ptr = absl::make_unique(tmp_dir); - } + auto cleanup_ptr = + do_extract ? absl::make_unique(tmp_dir) + : nullptr; std::string filename_absolute = MakeAbsolutePathAtCWD(filename); // Initialize sandbox and api objects. SapiLibarchiveSandboxExtract sandbox(filename_absolute, do_extract, tmp_dir); - CHECK(sandbox.Init().ok()) << "Error during sandbox initialization"; + SAPI_RETURN_IF_ERROR(sandbox.Init()); LibarchiveApi api(&sandbox); - archive* ret = api.archive_read_new().value(); - CHECK(ret != NULL) << "Failed to create read archive"; + SAPI_ASSIGN_OR_RETURN(archive * ret_archive, api.archive_read_new()); + if (ret_archive == nullptr) { + return absl::FailedPreconditionError("Failed to create read archive"); + } - sapi::v::RemotePtr a(ret); + sapi::v::RemotePtr a(ret_archive); - ret = api.archive_write_disk_new().value(); - CHECK(ret != NULL) << "Failed to create write disk archive"; + SAPI_ASSIGN_OR_RETURN(ret_archive, api.archive_write_disk_new()); + if (ret_archive == nullptr) { + return absl::FailedPreconditionError("Failed to create write disk archive"); + } - sapi::v::RemotePtr ext(ret); + sapi::v::RemotePtr ext(ret_archive); - int ret2; - CHECK(api.archive_write_disk_set_options(&ext, flags).value() == ARCHIVE_OK) - << "Unexpected result from write_disk_set_options call"; + int rc; + std::string msg; - CHECK(api.archive_read_support_filter_bzip2(&a).value() == ARCHIVE_OK) - << "Unexpected result from read_support_filter_bzip2 call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_disk_set_options(&ext, flags)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_disk_set_options call"); + } - CHECK(api.archive_read_support_filter_gzip(&a).value() == ARCHIVE_OK) - << "Unexpected result from read_suppport_filter_gzip call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_support_filter_bzip2(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from read_support_filter_bzip2 call"); + } - CHECK(api.archive_read_support_filter_compress(&a).value() == ARCHIVE_OK) - << "Unexpected result from read_support_filter_compress call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_support_filter_gzip(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from read_suppport_filter_gzip call"); + } - CHECK(api.archive_read_support_format_tar(&a).value() == ARCHIVE_OK) - << "Unexpected result fromread_support_format_tar call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_support_filter_compress(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from read_support_filter_compress call"); + } - CHECK(api.archive_read_support_format_cpio(&a).value() == ARCHIVE_OK) - << "Unexpected result from read_support_format_tar call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_support_format_tar(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result fromread_support_format_tar call"); + } - CHECK(api.archive_write_disk_set_standard_lookup(&ext).value() == ARCHIVE_OK) - << "Unexpected result from write_disk_set_standard_lookup call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_support_format_cpio(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from read_support_format_tar call"); + } + + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_disk_set_standard_lookup(&ext)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_disk_set_standard_lookup call"); + } const char* filename_ptr = filename_absolute.c_str(); - if (filename_ptr != NULL && strcmp(filename_ptr, "-") == 0) { - filename_ptr = NULL; + if (filename_ptr != nullptr && strcmp(filename_ptr, "-") == 0) { + filename_ptr = nullptr; } // The entries are saved with a relative path so they are all created // relative to the current working directory. - CHECK(api.archive_read_open_filename( - &a, sapi::v::ConstCStr(filename_ptr).PtrBefore(), kBlockSize) - .value() == ARCHIVE_OK) - << CheckStatusAndGetString(api.archive_error_string(&a), sandbox); + SAPI_ASSIGN_OR_RETURN( + rc, api.archive_read_open_filename( + &a, sapi::v::ConstCStr(filename_ptr).PtrBefore(), kBlockSize)); + if (rc != ARCHIVE_OK) { + SAPI_ASSIGN_OR_RETURN( + msg, CheckStatusAndGetString(api.archive_error_string(&a), sandbox)); + return absl::FailedPreconditionError(msg); + } - for (;;) { + while (true) { sapi::v::IntBase entry_ptr_tmp(0); - ret2 = api.archive_read_next_header(&a, entry_ptr_tmp.PtrAfter()).value(); + SAPI_ASSIGN_OR_RETURN( + rc, api.archive_read_next_header(&a, entry_ptr_tmp.PtrAfter())); - if (ret2 == ARCHIVE_EOF) { + if (rc == ARCHIVE_EOF) { break; } - CHECK(ret2 == ARCHIVE_OK) - << CheckStatusAndGetString(api.archive_error_string(&a), sandbox); + if (rc != ARCHIVE_OK) { + SAPI_ASSIGN_OR_RETURN( + msg, CheckStatusAndGetString(api.archive_error_string(&a), sandbox)); + return absl::FailedPreconditionError(msg); + } sapi::v::RemotePtr entry(entry_ptr_tmp.GetValue()); @@ -332,68 +430,92 @@ void extract(const char* filename, int do_extract, int flags, bool verbose) { } if (verbose || !do_extract) { - std::cout << CheckStatusAndGetString(api.archive_entry_pathname(&entry), - sandbox) - << std::endl; + SAPI_ASSIGN_OR_RETURN( + msg, + CheckStatusAndGetString(api.archive_entry_pathname(&entry), sandbox)); + std::cout << msg << std::endl; } if (do_extract) { - ret2 = api.archive_write_header(&ext, &entry).value(); + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_header(&ext, &entry)); - if (ret2 != ARCHIVE_OK) { - std::cout << CheckStatusAndGetString(api.archive_error_string(&a), - sandbox); + if (rc != ARCHIVE_OK) { + SAPI_ASSIGN_OR_RETURN(msg, CheckStatusAndGetString( + api.archive_error_string(&a), sandbox)); + std::cout << msg << std::endl; } else { - copy_data(&a, &ext, api, sandbox); + SAPI_ASSIGN_OR_RETURN(rc, CopyData(&a, &ext, api, sandbox)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Failed to copy data between archive structs."); + } } } } - CHECK(api.archive_read_close(&a).value() == ARCHIVE_OK) - << "Unexpected value from read_close call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_close(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected value from read_close call"); + } - CHECK(api.archive_read_free(&a).value() == ARCHIVE_OK) - << "Unexpected result from read_free call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_read_free(&a)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from read_free call"); + } - CHECK(api.archive_write_close(&ext).value() == ARCHIVE_OK) - << "Unexpected result from write_close call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_close(&ext)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_close call"); + } - CHECK(api.archive_write_free(&ext).value() == ARCHIVE_OK) - << "Unexpected result from write_free call"; + SAPI_ASSIGN_OR_RETURN(rc, api.archive_write_free(&ext)); + if (rc != ARCHIVE_OK) { + return absl::FailedPreconditionError( + "Unexpected result from write_free call"); + } + + return absl::OkStatus(); } -int copy_data(sapi::v::RemotePtr* ar, sapi::v::RemotePtr* aw, - LibarchiveApi& api, SapiLibarchiveSandboxExtract& sandbox) { - int ret; +absl::StatusOr CopyData(sapi::v::RemotePtr* ar, sapi::v::RemotePtr* aw, + LibarchiveApi& api, + SapiLibarchiveSandboxExtract& sandbox) { + int rc; + std::string msg; sapi::v::IntBase buff_ptr_tmp(0); sapi::v::ULLong size; sapi::v::SLLong offset; - for (;;) { - ret = api.archive_read_data_block(ar, buff_ptr_tmp.PtrAfter(), - size.PtrAfter(), offset.PtrAfter()) - .value(); + while (true) { + SAPI_ASSIGN_OR_RETURN( + rc, api.archive_read_data_block(ar, buff_ptr_tmp.PtrAfter(), + size.PtrAfter(), offset.PtrAfter())); - if (ret == ARCHIVE_EOF) { + if (rc == ARCHIVE_EOF) { return ARCHIVE_OK; } - if (ret != ARCHIVE_OK) { - std::cout << CheckStatusAndGetString(api.archive_error_string(ar), - sandbox); - return ret; + if (rc != ARCHIVE_OK) { + SAPI_ASSIGN_OR_RETURN( + msg, CheckStatusAndGetString(api.archive_error_string(ar), sandbox)); + std::cout << msg << std::endl; + return rc; } sapi::v::RemotePtr buff(buff_ptr_tmp.GetValue()); - ret = api.archive_write_data_block(aw, &buff, size.GetValue(), - offset.GetValue()) - .value(); + SAPI_ASSIGN_OR_RETURN( + rc, api.archive_write_data_block(aw, &buff, size.GetValue(), + offset.GetValue())); - if (ret != ARCHIVE_OK) { - std::cout << CheckStatusAndGetString(api.archive_error_string(ar), - sandbox); - return ret; + if (rc != ARCHIVE_OK) { + SAPI_ASSIGN_OR_RETURN( + msg, CheckStatusAndGetString(api.archive_error_string(ar), sandbox)); + std::cout << msg << std::endl; + return rc; } } } @@ -405,21 +527,20 @@ std::string MakeAbsolutePathAtCWD(const std::string& path) { return sandbox2::file::CleanPath(result); } -std::string CheckStatusAndGetString(const absl::StatusOr& status, - LibarchiveSandbox& sandbox) { - char* str = status.value(); - CHECK(str != NULL) << "Could not get error message"; - return sandbox.GetCString(sapi::v::RemotePtr(str)).value(); +absl::StatusOr CheckStatusAndGetString( + const absl::StatusOr& status, LibarchiveSandbox& sandbox) { + SAPI_ASSIGN_OR_RETURN(char* str, status); + if (str == nullptr) { + return absl::FailedPreconditionError("Could not get string from archive"); + } + return sandbox.GetCString(sapi::v::RemotePtr(str)); } -std::string CreateTempDirAtCWD() { +absl::StatusOr CreateTempDirAtCWD() { std::string cwd = sandbox2::file_util::fileops::GetCWD(); CHECK(!cwd.empty()) << "Could not get current working directory"; cwd.append("/"); - // We can manually check for .ok() result in this case because it offers - // important debugging information. - absl::StatusOr result = sandbox2::CreateTempDir(cwd); - CHECK(result.ok()) << "Could not create temporary directory at " << cwd; - return result.value(); + SAPI_ASSIGN_OR_RETURN(std::string result, sandbox2::CreateTempDir(cwd)); + return result; } diff --git a/oss-internship-2020/libarchive/examples/sapi_minitar.h b/oss-internship-2020/libarchive/examples/sapi_minitar.h index c881c21..7ab2b60 100644 --- a/oss-internship-2020/libarchive/examples/sapi_minitar.h +++ b/oss-internship-2020/libarchive/examples/sapi_minitar.h @@ -19,26 +19,27 @@ #include #include -#include "libarchive_sapi.sapi.h" -#include "sandbox.h" +#include "libarchive_sapi.sapi.h" // NOLINT(build/include) +#include "sandbox.h" // NOLINT(build/include) #include "sandboxed_api/sandbox2/util.h" #include "sandboxed_api/sandbox2/util/path.h" #include "sandboxed_api/sandbox2/util/temp_file.h" // Creates an archive file at the given filename. -void create(const char* filename, int compress, const char** argv, - bool verbose = true); +absl::Status CreateArchive(const char* filename, int compress, + const char** argv, bool verbose = true); // Extracts an archive file. If do_extract is true, the files will // be created relative to the current working directory. If do_extract // is false then the function will just print the entries of the archive. -void extract(const char* filename, int do_extract, int flags, - bool verbose = true); +absl::Status ExtractArchive(const char* filename, int do_extract, int flags, + bool verbose = true); // This function is only called from the "extract function". It is still // isolated in order to not modify the code structure as much. -int copy_data(sapi::v::RemotePtr* ar, sapi::v::RemotePtr* aw, - LibarchiveApi& api, SapiLibarchiveSandboxExtract& sandbox); +absl::StatusOr CopyData(sapi::v::RemotePtr* ar, sapi::v::RemotePtr* aw, + LibarchiveApi& api, + SapiLibarchiveSandboxExtract& sandbox); inline constexpr size_t kBlockSize = 10240; inline constexpr size_t kBuffSize = 16384; @@ -51,12 +52,12 @@ std::string MakeAbsolutePathAtCWD(const std::string& path); // This function takes a status as argument and after checking the status // it transfers the string. This is used mostly with archive_error_string // and other library functions that return a char*. -std::string CheckStatusAndGetString(const absl::StatusOr& status, - LibarchiveSandbox& sandbox); +absl::StatusOr CheckStatusAndGetString( + const absl::StatusOr& status, LibarchiveSandbox& sandbox); // Creates a temporary directory in the current working directory and // returns the path. This is used in the extract function where the sandboxed // process changes the current working directory to this temporary directory. -std::string CreateTempDirAtCWD(); +absl::StatusOr CreateTempDirAtCWD(); #endif // SAPI_LIBARCHIVE_EXAMPLES_MINITAR_H diff --git a/oss-internship-2020/libarchive/examples/sapi_minitar_main.cc b/oss-internship-2020/libarchive/examples/sapi_minitar_main.cc index c323600..6eddf94 100644 --- a/oss-internship-2020/libarchive/examples/sapi_minitar_main.cc +++ b/oss-internship-2020/libarchive/examples/sapi_minitar_main.cc @@ -20,22 +20,38 @@ #include -#include "sapi_minitar.h" +#include "sapi_minitar.h" // NOLINT(build/include) -static void usage(void); +static void PrintUsage() { + /* Many program options depend on compile options. */ + const char* m = + "Usage: minitar [-" + "c" + "j" + "tvx" + "y" + "Z" + "z" + "] [-f file] [file]\n"; -int main(int argc, const char** argv) { + std::cout << m << std::endl; + exit(EXIT_FAILURE); +} + +int main(int unused_argc, const char** argv) { google::InitGoogleLogging(argv[0]); - const char* filename = NULL; - int compress, flags, mode, opt; + const char* filename = nullptr; + int compress; + int flags; + int mode; + int opt; - (void)argc; mode = 'x'; int verbose = 0; compress = '\0'; flags = ARCHIVE_EXTRACT_TIME; - while (*++argv != NULL && **argv == '-') { + while (*++argv != nullptr && **argv == '-') { const char* p = *argv + 1; while ((opt = *p++) != '\0') { @@ -77,38 +93,38 @@ int main(int argc, const char** argv) { compress = opt; break; default: - usage(); + PrintUsage(); } } } + absl::Status status; switch (mode) { case 'c': - create(filename, compress, argv, verbose); + status = CreateArchive(filename, compress, argv, verbose); + if (!status.ok()) { + LOG(ERROR) << "Archive creation failed with message: " + << status.message(); + return EXIT_FAILURE; + } break; case 't': - extract(filename, 0, flags, verbose); + status = ExtractArchive(filename, 0, flags, verbose); + if (!status.ok()) { + LOG(ERROR) << "Archive extraction failed with message: " + << status.message(); + return EXIT_FAILURE; + } break; case 'x': - extract(filename, 1, flags, verbose); + status = ExtractArchive(filename, 1, flags, verbose); + if (!status.ok()) { + LOG(ERROR) << "Archive extraction failed with message: " + << status.message(); + return EXIT_FAILURE; + } break; } return EXIT_SUCCESS; } - -static void usage(void) { - /* Many program options depend on compile options. */ - const char* m = - "Usage: minitar [-" - "c" - "j" - "tvx" - "y" - "Z" - "z" - "] [-f file] [file]\n"; - - std::cout << m << std::endl; - exit(EXIT_FAILURE); -} diff --git a/oss-internship-2020/libarchive/test/CMakeLists.txt b/oss-internship-2020/libarchive/test/CMakeLists.txt index 50035e9..1e46868 100644 --- a/oss-internship-2020/libarchive/test/CMakeLists.txt +++ b/oss-internship-2020/libarchive/test/CMakeLists.txt @@ -25,4 +25,4 @@ target_link_libraries(sapi_minitar_test PRIVATE sapi::test_main ) -gtest_discover_tests(sapi_minitar_test) \ No newline at end of file +gtest_discover_tests(sapi_minitar_test) diff --git a/oss-internship-2020/libarchive/test/minitar_test.cc b/oss-internship-2020/libarchive/test/minitar_test.cc index 255c566..71553ef 100644 --- a/oss-internship-2020/libarchive/test/minitar_test.cc +++ b/oss-internship-2020/libarchive/test/minitar_test.cc @@ -14,13 +14,13 @@ #include -#include "build/googletest-src/googlemock/include/gmock/gmock-more-matchers.h" #include "gtest/gtest.h" #include "sandboxed_api/sandbox2/util/path.h" #include "sandboxed_api/util/status_matchers.h" -#include "sapi_minitar.h" +#include "sapi_minitar.h" // NOLINT(build/include) using ::sandbox2::file::JoinPath; +using ::sapi::IsOk; using ::testing::Eq; using ::testing::IsTrue; using ::testing::StrEq; @@ -47,7 +47,10 @@ class MiniTarTest : public ::testing::Test { // -dir1 - file2 // - dir2 - file3 static void SetUpTestSuite() { - data_dir_ = CreateTempDirAtCWD(); + absl::StatusOr tmp_status = CreateTempDirAtCWD(); + ASSERT_THAT(tmp_status, IsOk()); + data_dir_ = tmp_status.value(); + init_wd_ = sandbox2::file_util::fileops::GetCWD(); ASSERT_THAT(Exists(data_dir_, false), IsTrue()) << "Test data directory was not created"; @@ -78,7 +81,11 @@ class MiniTarTest : public ::testing::Test { // We use a unique id based on test count to make sure that files created // during tests do not overlap. id_ = "test" + std::to_string(test_count_); - tmp_dir_ = CreateTempDirAtCWD(); + + absl::StatusOr tmp_status = CreateTempDirAtCWD(); + ASSERT_THAT(tmp_status, IsOk()); + tmp_dir_ = tmp_status.value(); + ASSERT_THAT(Exists(tmp_dir_, false), IsTrue) << "Could not create test specific temporary directory"; ASSERT_THAT(chdir(data_dir_.data()), Eq(0)) @@ -153,26 +160,30 @@ std::string MiniTarTest::init_wd_; TEST_F(MiniTarTest, TestFileSimple) { std::vector v = {kFile1_.data()}; - create(id_.data(), 0, VecStringToCharPtrArr(v), false); + ASSERT_THAT(CreateArchive(id_.data(), 0, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile1_)); } TEST_F(MiniTarTest, TestMultipleFiles) { std::vector v = {kFile1_.data(), kFile2_.data(), kFile3_.data()}; - create(id_.data(), 0, VecStringToCharPtrArr(v), false); + ASSERT_THAT(CreateArchive(id_.data(), 0, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(Exists(id_.data(), false), IsTrue()) << "Archive file was not created"; ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile1_)); CheckFile(std::string(kFile2_)); @@ -181,24 +192,28 @@ TEST_F(MiniTarTest, TestMultipleFiles) { TEST_F(MiniTarTest, TestDirectorySimple) { std::vector v = {kDir2_.data()}; - create(id_.data(), 0, VecStringToCharPtrArr(v), false); + ASSERT_THAT(CreateArchive(id_.data(), 0, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile3_)); } TEST_F(MiniTarTest, TestDirectoryNested) { std::vector v = {kDir1_.data()}; - create(id_.data(), 0, VecStringToCharPtrArr(v), false); + ASSERT_THAT(CreateArchive(id_.data(), 0, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile2_)); CheckFile(std::string(kFile3_)); @@ -206,12 +221,14 @@ TEST_F(MiniTarTest, TestDirectoryNested) { TEST_F(MiniTarTest, TestComplex) { std::vector v = {kFile1_.data(), kDir1_.data()}; - create(id_.data(), 0, VecStringToCharPtrArr(v), false); + ASSERT_THAT(CreateArchive(id_.data(), 0, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile1_)); CheckFile(std::string(kFile2_)); @@ -221,11 +238,14 @@ TEST_F(MiniTarTest, TestComplex) { TEST_F(MiniTarTest, TestCompress) { std::vector v = {kFile1_.data(), kDir1_.data()}; int compress = 'Z'; - create(id_.data(), compress, VecStringToCharPtrArr(v), false); + ASSERT_THAT( + CreateArchive(id_.data(), compress, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile1_)); CheckFile(std::string(kFile2_)); @@ -235,11 +255,14 @@ TEST_F(MiniTarTest, TestCompress) { TEST_F(MiniTarTest, TestGZIP) { std::vector v = {kFile1_.data(), kDir1_.data()}; int compress = 'z'; - create(id_.data(), compress, VecStringToCharPtrArr(v), false); + ASSERT_THAT( + CreateArchive(id_.data(), compress, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile1_)); CheckFile(std::string(kFile2_)); @@ -249,11 +272,14 @@ TEST_F(MiniTarTest, TestGZIP) { TEST_F(MiniTarTest, TestBZIP2) { std::vector v = {kFile1_.data(), kDir1_.data()}; int compress = 'j'; - create(id_.data(), compress, VecStringToCharPtrArr(v), false); + ASSERT_THAT( + CreateArchive(id_.data(), compress, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile1_)); CheckFile(std::string(kFile2_)); @@ -264,11 +290,13 @@ TEST_F(MiniTarTest, TestPaths) { // These should be equivalent to kFile1_ and kDir1_ after cleaning. std::vector v = {JoinPath("a/b/../../c/../", kFile1_).data(), JoinPath("d/../e/././///../", kDir1_).data()}; - create(id_.data(), 0, VecStringToCharPtrArr(v), false); + ASSERT_THAT(CreateArchive(id_.data(), 0, VecStringToCharPtrArr(v), false), + IsOk()); ASSERT_THAT(chdir(tmp_dir_.data()), Eq(0)) << "Could not chdir into test data directory"; - extract(JoinPath(data_dir_, id_).data(), 1, 0, false); + ASSERT_THAT(ExtractArchive(JoinPath(data_dir_, id_).data(), 1, 0, false), + IsOk()); CheckFile(std::string(kFile1_)); CheckFile(std::string(kFile2_));