A seek on a closed channel can allow invalid inputs

[shym]

While running some qcheck-lin tests of In_channel, I ran into a problem which boils down to this test:

let _ =
  let f = open_in Sys.argv.(0) in
  seek_in f 1 ;
  close_in_noerr f ;
  seek_in f 0 ;
  Printf.printf "Read: %d\n%!" (input_byte f) ;
  Printf.printf "Read: %d\n%!" (input_byte f)

yielding:

Read: 0
Fatal error: exception Sys_error("Bad file descriptor")
Raised by primitive operation at Seekextra in file "seekextra.ml", line 7, characters 31-45

Replacing the first seek_in by seek_in f 10 would allow inputting 10 bytes before getting an exception.
In this example, the extra byte(s) we input were never really read from the file, they are what’s currently in the buffer (which is not initialized, I think, which would explain that random testing it could find such a code giving non-deterministic results).

As I understand caml_ml_close_channel, we want to avoid testing explicitly whether the channel is indeed open in every function and rather rely on failing syscalls to detect it. So I checked that simply resetting the offset when closing the file:

@@ -681,6 +681,7 @@ CAMLprim value caml_ml_close_channel(value vchannel)
      immediate caml_flush_partial or caml_refill, thus raising a Sys_error
      exception */
   channel->curr = channel->max = channel->end;
+  channel->offset = 0;
 
   /* If already closed, we are done */
   if (channel->fd != -1) {

is enough to get an exception on the first input after closing. It does still allow for the seek_in on the closed channel, so that’s maybe not the best way to go.

[xavierleroy]

Interesting example, thanks! I'm surprised that "the extra byte(s) we input were never really read from the file". Seeking can move within data already read in the buffer, but should not expose parts of the buffer that have not been read. Maybe this is the root of the problem, independently of whether the channel is closed between the two seek_in.

[shym]

Here is, step by step, my understanding of what happens in that example.
First we open the file with caml_open_descriptor_in, offset is set to 0, curr and max point to the beginning of the buffer.

ocaml/runtime/io.c

Lines 171 to 174 in d70eae0

	channel->offset = lseek(fd, 0, SEEK_CUR);
	caml_leave_blocking_section();
	channel->curr = channel->max = channel->buff;
	channel->end = channel->buff + IO_BUFFER_SIZE;

Then we seek to position 1 with caml_seek_in, offset is 1, curr and max are assigned again at the beginning of the buffer.

ocaml/runtime/io.c

Lines 437 to 443 in d70eae0

	if (lseek(channel->fd, dest, SEEK_SET) != dest) {
	caml_leave_blocking_section();
	caml_sys_error(NO_ARG);
	}
	caml_leave_blocking_section();
	channel->offset = dest;
	channel->curr = channel->max = channel->buff;

Then we close the channel with caml_ml_close_channel, curr and max are set to the end of the buffer but, critically, offset is not modified (so keep at 1).

ocaml/runtime/io.c

Lines 680 to 683 in d70eae0

	/* Ensure that every read or write on the channel will cause an
	immediate caml_flush_partial or caml_refill, thus raising a Sys_error
	exception */
	channel->curr = channel->max = channel->end;

Then we seek back with caml_seek_in, but this time the condition is true, the function assumes that we are seeking inside data in the buffer (note though that no read ever happened), so curr points now to the last byte of the buffer.

ocaml/runtime/io.c

Lines 431 to 434 in d70eae0

	if (dest >= channel->offset - (channel->max - channel->buff)
	&& dest <= channel->offset
	&& (channel->flags & CHANNEL_TEXT_MODE) == 0) {
	channel->curr = channel->max - (channel->offset - dest);

Then the next input will read that byte, because it is deemed valid.

[xavierleroy]

Thanks for the step-by-step. It's now clear in my mind that the problem is with close_in.