os: fix file read end-of-file detection (#10070)

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<T>(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<T>(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>() ?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<T>(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<T>(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<T>(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<T>(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))
 		}

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))

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()