Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent seek_in from marking buffer data as valid after closing the channel (Fixes #11878) #11965

Merged
merged 2 commits into from Feb 7, 2023

Conversation

shym
Copy link
Contributor

@shym shym commented Jan 26, 2023

This proposes a fix for issue #11878 in which some part of a in_channel buffer can be considered as valid after the channel has been closed.
This works by simply resetting the channel offset to 0 when the channel is closed to disable any backward seek_in later.
This PR also introduces a test for that issue (before and with the fix).

Closes issue #11878

@OlivierNicole
Copy link
Contributor

The test you introduced fails on Windows. Maybe lseek behaves differently there?

@xavierleroy
Copy link
Contributor

Better use open_in_bin instead of open_in in the test. Seeking on files opened in text mode under Windows generally doesn't do what you want.

If offset keeps its value when the channel is closed, it is possible to
`seek_in` backward which will only decrease `channel->curr` and therefore
mark some buffer bytes as valid: further calls to `input_byte` would
succeed on the closed channel

Fix issue ocaml#11878
@shym
Copy link
Contributor Author

shym commented Jan 27, 2023

Thank you for the tip!
I hope I correctly handled the other CI remarks while I was updating the branch.

Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. The change makes complete sense and there's a test.

@xavierleroy xavierleroy merged commit a606e92 into ocaml:trunk Feb 7, 2023
@gasche
Copy link
Member

gasche commented Jan 29, 2024

I'm trying to understand this weird offset = 0 assignment when closing channels in the context of #12898.

Naive question: why don't we fix the present issue by failing on seek_in on a closed channel? Is there any issue with that approach?
There is a fast path in caml_seek_in, where the position we seek is available in the input buffer, I propose to fail in that case (as in the other case) when the file descriptor is invalid.

This requires changing the test introduced in the present PR,

  let ic = open_in_bin Sys.argv.(0) in
  seek_in ic nb_bytes;
  close_in ic;
  seek_in ic 0;
  assert (
    try
      ignore (input_byte ic);
      false
    with
    | Sys_error _ -> true
    | _           -> false)

to move the seek_in ic 0 inside the try .. with block, because now the code fails at seek_in time rather than at input_byte.

gasche added a commit to gasche/ocaml that referenced this pull request Jan 29, 2024
This reverts a technical decision made in ocaml#11878, ocaml#11965, for the
purpose of simplifying the implementation of [close_in] and the
specification of [seek_in], which now consistently fails on closed
channels.
edwintorok pushed a commit to edwintorok/ocaml that referenced this pull request Feb 10, 2024
… channel (ocaml#11965)

* Test calls to input_byte after close_in

Add a test demonstrating issue ocaml#11878

* Reset offset when closing a channel

If offset keeps its value when the channel is closed, it is possible to
`seek_in` backward which will only decrease `channel->curr` and therefore
mark some buffer bytes as valid: further calls to `input_byte` would
succeed on the closed channel

Fixes: ocaml#11878
(cherry picked from commit a606e92)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants