From 49deeac71e540bcab52ad97cbcb448bd0ffa1c22 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch Date: Wed, 12 May 2021 23:48:55 -0700 Subject: [PATCH] os: fix file read end-of-file detection (#10070) --- vlib/os/file.c.v | 100 +++++++++++++++++--------------------------- vlib/os/file_test.v | 64 ++++++++++++++++++++++++++++ vlib/os/os_test.v | 9 +++- 3 files changed, 110 insertions(+), 63 deletions(-) diff --git a/vlib/os/file.c.v b/vlib/os/file.c.v index cdc612db77..6f2af20bb1 100644 --- a/vlib/os/file.c.v +++ b/vlib/os/file.c.v @@ -321,6 +321,31 @@ pub fn (mut f File) write_ptr_at(data voidptr, size int, pos u64) int { } // **************************** Read ops *************************** + +// fread wraps C.fread and handles error and end-of-file detection. +fn fread(ptr voidptr, item_size int, items int, stream &C.FILE) ?int { + nbytes := int(C.fread(ptr, item_size, items, stream)) + // If no bytes were read, check for errors and end-of-file. + if nbytes <= 0 { + // If fread encountered end-of-file return the none error. Note that fread + // may read data and encounter the end-of-file, but we shouldn't return none + // in that case which is why we only check for end-of-file if no data was + // read. The caller will get none on their next call because there will be + // no data available and the end-of-file will be encountered again. + if C.feof(stream) != 0 { + return none + } + // If fread encountered an error, return it. Note that fread and ferror do + // not tell us what the error was, so we can't return anything more specific + // than there was an error. This is because fread and ferror do not set + // errno. + if C.ferror(stream) != 0 { + return error('file read error') + } + } + return nbytes +} + // read_bytes reads bytes from the beginning of the file. // Utility method, same as .read_bytes_at(size, 0). pub fn (f &File) read_bytes(size int) []byte { @@ -348,23 +373,14 @@ pub fn (f &File) read_bytes_into(pos u64, mut buf []byte) ?int { $if windows { // Note: fseek errors if pos == os.file_size, which we accept C._fseeki64(f.cfile, pos, C.SEEK_SET) - // errno is only set if fread fails, so clear it first to tell - C.errno = 0 - nbytes := int(C.fread(buf.data, 1, buf.len, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes := fread(buf.data, 1, buf.len, f.cfile) ? $if debug { C._fseeki64(f.cfile, 0, C.SEEK_SET) } return nbytes } $else { C.fseeko(f.cfile, pos, C.SEEK_SET) - C.errno = 0 - nbytes := int(C.fread(buf.data, 1, buf.len, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes := fread(buf.data, 1, buf.len, f.cfile) ? $if debug { C.fseeko(f.cfile, 0, C.SEEK_SET) } @@ -373,11 +389,7 @@ pub fn (f &File) read_bytes_into(pos u64, mut buf []byte) ?int { } $if x32 { C.fseek(f.cfile, pos, C.SEEK_SET) - C.errno = 0 - nbytes := int(C.fread(buf.data, 1, buf.len, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes := fread(buf.data, 1, buf.len, f.cfile) ? $if debug { C.fseek(f.cfile, 0, C.SEEK_SET) } @@ -391,11 +403,7 @@ pub fn (f &File) read(mut buf []byte) ?int { if buf.len == 0 { return 0 } - C.errno = 0 - nbytes := int(C.fread(buf.data, 1, buf.len, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes := fread(buf.data, 1, buf.len, f.cfile) ? return nbytes } @@ -417,20 +425,12 @@ pub fn (f &File) read_from(pos u64, mut buf []byte) ?int { C.fseeko(f.cfile, pos, C.SEEK_SET) } - C.errno = 0 - nbytes := int(C.fread(buf.data, 1, buf.len, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes := fread(buf.data, 1, buf.len, f.cfile) ? return nbytes } $if x32 { C.fseek(f.cfile, pos, C.SEEK_SET) - C.errno = 0 - nbytes := int(C.fread(buf.data, 1, buf.len, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes := fread(buf.data, 1, buf.len, f.cfile) ? return nbytes } return error('Could not read file') @@ -461,11 +461,7 @@ pub fn (mut f File) read_struct(mut t T) ? { if tsize == 0 { return none } - C.errno = 0 - nbytes := int(C.fread(t, 1, tsize, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes := fread(t, 1, tsize, f.cfile) ? if nbytes != tsize { return error_with_code('incomplete struct read', nbytes) } @@ -480,27 +476,23 @@ pub fn (mut f File) read_struct_at(mut t T, pos u64) ? { if tsize == 0 { return none } - C.errno = 0 mut nbytes := 0 $if x64 { $if windows { C._fseeki64(f.cfile, pos, C.SEEK_SET) - nbytes = int(C.fread(t, 1, tsize, f.cfile)) + nbytes = fread(t, 1, tsize, f.cfile) ? C._fseeki64(f.cfile, 0, C.SEEK_END) } $else { C.fseeko(f.cfile, pos, C.SEEK_SET) - nbytes = int(C.fread(t, 1, tsize, f.cfile)) + nbytes = fread(t, 1, tsize, f.cfile) ? C.fseeko(f.cfile, 0, C.SEEK_END) } } $if x32 { C.fseek(f.cfile, pos, C.SEEK_SET) - nbytes = int(C.fread(t, 1, tsize, f.cfile)) + nbytes = fread(t, 1, tsize, f.cfile) ? C.fseek(f.cfile, 0, C.SEEK_END) } - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } if nbytes != tsize { return error_with_code('incomplete struct read', nbytes) } @@ -515,12 +507,8 @@ pub fn (mut f File) read_raw() ?T { if tsize == 0 { return none } - C.errno = 0 mut t := T{} - nbytes := int(C.fread(&t, 1, tsize, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes := fread(&t, 1, tsize, f.cfile) ? if nbytes != tsize { return error_with_code('incomplete struct read', nbytes) } @@ -536,7 +524,6 @@ pub fn (mut f File) read_raw_at(pos u64) ?T { if tsize == 0 { return none } - C.errno = 0 mut nbytes := 0 mut t := T{} $if x64 { @@ -544,10 +531,7 @@ pub fn (mut f File) read_raw_at(pos u64) ?T { if C._fseeki64(f.cfile, pos, C.SEEK_SET) != 0 { return error(posix_get_error_msg(C.errno)) } - nbytes = int(C.fread(&t, 1, tsize, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes = fread(&t, 1, tsize, f.cfile) ? if C._fseeki64(f.cfile, 0, C.SEEK_END) != 0 { return error(posix_get_error_msg(C.errno)) } @@ -555,10 +539,7 @@ pub fn (mut f File) read_raw_at(pos u64) ?T { if C.fseeko(f.cfile, pos, C.SEEK_SET) != 0 { return error(posix_get_error_msg(C.errno)) } - nbytes = int(C.fread(&t, 1, tsize, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes = fread(&t, 1, tsize, f.cfile) ? if C.fseeko(f.cfile, 0, C.SEEK_END) != 0 { return error(posix_get_error_msg(C.errno)) } @@ -568,10 +549,7 @@ pub fn (mut f File) read_raw_at(pos u64) ?T { if C.fseek(f.cfile, pos, C.SEEK_SET) != 0 { return error(posix_get_error_msg(C.errno)) } - nbytes = int(C.fread(&t, 1, tsize, f.cfile)) - if C.errno != 0 { - return error(posix_get_error_msg(C.errno)) - } + nbytes = fread(&t, 1, tsize, f.cfile) ? if C.fseek(f.cfile, 0, C.SEEK_END) != 0 { return error(posix_get_error_msg(C.errno)) } diff --git a/vlib/os/file_test.v b/vlib/os/file_test.v index 4e8616a966..c8ab607c7d 100644 --- a/vlib/os/file_test.v +++ b/vlib/os/file_test.v @@ -59,6 +59,70 @@ fn testsuite_end() ? { assert !os.is_dir(tfolder) } +// test_read_eof_last_read_partial_buffer_fill tests that when reading a file +// the end-of-file is detected and results in a none error being returned. This +// test simulates file reading where the end-of-file is reached inside an fread +// containing data. +fn test_read_eof_last_read_partial_buffer_fill() ? { + mut f := os.open_file(tfile, 'w') ? + bw := []byte{len: 199, init: 5} + f.write(bw) ? + f.close() + + f = os.open_file(tfile, 'r') ? + mut br := []byte{len: 100} + // Read first 100 bytes of 199 byte file, should fill buffer with no error. + n0 := f.read(mut br) ? + assert n0 == 100 + // Read remaining 99 bytes of 199 byte file, should fill buffer with no + // error, even though end-of-file was reached. + n1 := f.read(mut br) ? + assert n1 == 99 + // Read again, end-of-file was previously reached so should return none + // error. + if _ := f.read(mut br) { + // This is not intended behavior because the read function should + // not return a number of bytes read when end-of-file is reached. + assert false + } else { + // Expect none to have been returned when end-of-file. + assert err is none + } + f.close() +} + +// test_read_eof_last_read_full_buffer_fill tests that when reading a file the +// end-of-file is detected and results in a none error being returned. This test +// simulates file reading where the end-of-file is reached at the beinning of an +// fread that returns no data. +fn test_read_eof_last_read_full_buffer_fill() ? { + mut f := os.open_file(tfile, 'w') ? + bw := []byte{len: 200, init: 5} + f.write(bw) ? + f.close() + + f = os.open_file(tfile, 'r') ? + mut br := []byte{len: 100} + // Read first 100 bytes of 200 byte file, should fill buffer with no error. + n0 := f.read(mut br) ? + assert n0 == 100 + // Read remaining 100 bytes of 200 byte file, should fill buffer with no + // error. The end-of-file isn't reached yet, but there is no more data. + n1 := f.read(mut br) ? + assert n1 == 100 + // Read again, end-of-file was previously reached so should return none + // error. + if _ := f.read(mut br) { + // This is not intended behavior because the read function should + // not return a number of bytes read when end-of-file is reached. + assert false + } else { + // Expect none to have been returned when end-of-file. + assert err is none + } + f.close() +} + fn test_write_struct() ? { os.rm(tfile) or {} // FIXME This is a workaround for macos, because the file isn't truncated when open with 'w' size_of_point := int(sizeof(Point)) diff --git a/vlib/os/os_test.v b/vlib/os/os_test.v index ce324a74f6..a9c3813574 100644 --- a/vlib/os/os_test.v +++ b/vlib/os/os_test.v @@ -160,8 +160,13 @@ fn test_write_and_read_bytes() { // check that trying to read data from EOF doesn't error and returns 0 mut a := []byte{len: 5} nread := file_read.read_bytes_into(5, mut a) or { - eprintln(err) - int(-1) + n := if err is none { + int(0) + } else { + eprintln(err) + int(-1) + } + n } assert nread == 0 file_read.close()