Skip to content

Close reader descriptor after open errors#28

Open
jeet-dekivadia wants to merge 2 commits into
janestreet:masterfrom
jeet-dekivadia:close-reader-fd-after-open-error
Open

Close reader descriptor after open errors#28
jeet-dekivadia wants to merge 2 commits into
janestreet:masterfrom
jeet-dekivadia:close-reader-fd-after-open-error

Conversation

@jeet-dekivadia

@jeet-dekivadia jeet-dekivadia commented May 31, 2026

Copy link
Copy Markdown

Fixes #27.

Reader.open_ opened a descriptor and passed it directly into trace-header parsing. If a truncated or malformed trace raised during make_reader, the caller never received a reader value and had no descriptor to close.

Close the descriptor best-effort on the exception path and re-raise the original parsing exception. Successful readers retain ownership exactly as before.

The Linux regression opens an invalid trace ten times and checks /proc/self/fd before and after the loop, so the failure path must leave the descriptor count flat.

Validation:

  • git diff --check
  • private Linux runner: installed OCaml 5.2.1 and ppx_jane, then ran dune build @all -j2; compilation reaches the repository sources but the public toolchain cannot compile existing Domain.Safe and Sys.Safe uses and existing warning-as-error findings, so dune runtest is not reachable outside the Jane Street build environment

Signed-off-by: Jeet Dekivadia <jeet.university@gmail.com>
Signed-off-by: Jeet Dekivadia <jeet.university@gmail.com>
@github-iron github-iron added the forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system. label Jun 1, 2026
@jeet-dekivadia

Copy link
Copy Markdown
Author

Small follow-up on this one: the patch is limited to the Reader.open_ exception path where the descriptor has already been opened but no reader is returned to the caller. The cleanup is best-effort and re-raises the original parse error, so successful reader ownership is unchanged.

The regression exercises repeated malformed opens and checks that /proc/self/fd stays flat. DCO is green. Happy to adjust the local shape if you would prefer this cleanup to sit somewhere else in the open path.

@jeet-dekivadia

Copy link
Copy Markdown
Author

I rechecked this today while it is waiting for review. The diff is still narrowly on the Reader.open_ failure path after the file descriptor has been acquired and before successful reader ownership can be returned to the caller.

One detail I wanted to call out explicitly: the cleanup intentionally preserves the original parse/open exception and treats descriptor close as best-effort only on that failure path. The normal successful reader path is unchanged. DCO is still green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

forwarded-to-js-devs This report has been forwarded to Jane Street's internal review system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reader.open_ leaks its descriptor when trace header parsing fails

2 participants