From ea5518f69edc51e8e70c2c4d4c4daa3ad9bcb242 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 13 Apr 2019 12:31:45 +0200 Subject: [PATCH 1/2] make os_file_close poison file handle after close This helps track down use-after-close bugs. --- src/cache_hash.cpp | 24 ++++++++++++------------ src/os.cpp | 8 +++++--- src/os.hpp | 2 +- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/cache_hash.cpp b/src/cache_hash.cpp index 1f25a9982e..250733ec92 100644 --- a/src/cache_hash.cpp +++ b/src/cache_hash.cpp @@ -256,10 +256,10 @@ static Error populate_file_hash(CacheHash *ch, CacheHashFile *chf, Buf *contents } if ((err = hash_file(chf->bin_digest, this_file, contents))) { - os_file_close(this_file); + os_file_close(&this_file); return err; } - os_file_close(this_file); + os_file_close(&this_file); blake2b_update(&ch->blake, chf->bin_digest, 48); @@ -300,7 +300,7 @@ Error cache_hit(CacheHash *ch, Buf *out_digest) { Buf line_buf = BUF_INIT; buf_resize(&line_buf, 512); if ((err = os_file_read_all(ch->manifest_file, &line_buf))) { - os_file_close(ch->manifest_file); + os_file_close(&ch->manifest_file); return err; } @@ -389,14 +389,14 @@ Error cache_hit(CacheHash *ch, Buf *out_digest) { OsFileAttr actual_attr; if ((err = os_file_open_r(chf->path, &this_file, &actual_attr))) { fprintf(stderr, "Unable to open %s\n: %s", buf_ptr(chf->path), err_str(err)); - os_file_close(ch->manifest_file); + os_file_close(&ch->manifest_file); return ErrorCacheUnavailable; } if (chf->attr.mtime.sec == actual_attr.mtime.sec && chf->attr.mtime.nsec == actual_attr.mtime.nsec && chf->attr.inode == actual_attr.inode) { - os_file_close(this_file); + os_file_close(&this_file); } else { // we have to recompute the digest. // later we'll rewrite the manifest with the new mtime/digest values @@ -411,11 +411,11 @@ Error cache_hit(CacheHash *ch, Buf *out_digest) { uint8_t actual_digest[48]; if ((err = hash_file(actual_digest, this_file, nullptr))) { - os_file_close(this_file); - os_file_close(ch->manifest_file); + os_file_close(&this_file); + os_file_close(&ch->manifest_file); return err; } - os_file_close(this_file); + os_file_close(&this_file); if (memcmp(chf->bin_digest, actual_digest, 48) != 0) { memcpy(chf->bin_digest, actual_digest, 48); // keep going until we have the input file digests @@ -433,12 +433,12 @@ Error cache_hit(CacheHash *ch, Buf *out_digest) { CacheHashFile *chf = &ch->files.at(file_i); if ((err = populate_file_hash(ch, chf, nullptr))) { fprintf(stderr, "Unable to hash %s: %s\n", buf_ptr(chf->path), err_str(err)); - os_file_close(ch->manifest_file); + os_file_close(&ch->manifest_file); return ErrorCacheUnavailable; } } if (return_code != ErrorNone) { - os_file_close(ch->manifest_file); + os_file_close(&ch->manifest_file); } return return_code; } @@ -453,7 +453,7 @@ Error cache_add_file_fetch(CacheHash *ch, Buf *resolved_path, Buf *contents) { CacheHashFile *chf = ch->files.add_one(); chf->path = resolved_path; if ((err = populate_file_hash(ch, chf, contents))) { - os_file_close(ch->manifest_file); + os_file_close(&ch->manifest_file); return err; } @@ -586,6 +586,6 @@ void cache_release(CacheHash *ch) { } } - os_file_close(ch->manifest_file); + os_file_close(&ch->manifest_file); } diff --git a/src/os.cpp b/src/os.cpp index 470d222307..60c66908cc 100644 --- a/src/os.cpp +++ b/src/os.cpp @@ -2081,11 +2081,13 @@ Error os_file_overwrite(OsFile file, Buf *contents) { #endif } -void os_file_close(OsFile file) { +void os_file_close(OsFile *file) { #if defined(ZIG_OS_WINDOWS) - CloseHandle(file); + CloseHandle(*file); + *file = NULL; #else - close(file); + close(*file); + *file = -1; #endif } diff --git a/src/os.hpp b/src/os.hpp index 5064a6444c..b301937e83 100644 --- a/src/os.hpp +++ b/src/os.hpp @@ -121,7 +121,7 @@ Error ATTRIBUTE_MUST_USE os_file_open_lock_rw(Buf *full_path, OsFile *out_file); Error ATTRIBUTE_MUST_USE os_file_read(OsFile file, void *ptr, size_t *len); Error ATTRIBUTE_MUST_USE os_file_read_all(OsFile file, Buf *contents); Error ATTRIBUTE_MUST_USE os_file_overwrite(OsFile file, Buf *contents); -void os_file_close(OsFile file); +void os_file_close(OsFile *file); Error ATTRIBUTE_MUST_USE os_write_file(Buf *full_path, Buf *contents); Error ATTRIBUTE_MUST_USE os_copy_file(Buf *src_path, Buf *dest_path); From 93e89b3b7efeaa41f4f11fbb022b962d2244dab2 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 13 Apr 2019 12:33:29 +0200 Subject: [PATCH 2/2] don't close cache manifest file prematurely ErrorInvalidFormat is not a fatal error so don't close the cache manifest file right away but instead let cache_final() handle it. Fixes the following (very common) warning when running the test suite: Warning: Unable to write cache file [..]: unexpected seek failure The seek failure is an lseek() system call that failed with EBADF because the file descriptor had already been closed. --- src/cache_hash.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cache_hash.cpp b/src/cache_hash.cpp index 250733ec92..efb1a06b59 100644 --- a/src/cache_hash.cpp +++ b/src/cache_hash.cpp @@ -437,7 +437,7 @@ Error cache_hit(CacheHash *ch, Buf *out_digest) { return ErrorCacheUnavailable; } } - if (return_code != ErrorNone) { + if (return_code != ErrorNone && return_code != ErrorInvalidFormat) { os_file_close(&ch->manifest_file); } return return_code;