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

Fix Lwt_unix.lstat calling Unix.stat on Win32 #883

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

MisterDA
Copy link
Contributor

A nice little bug here ;)

It can become rather nasty with this code (how I found it):

  Lwt_main.run begin
      with_temp_dir @@ fun dir ->
      let link = Filename.(concat dir "link") in
      Unix.symlink ~to_dir:true "C:/Windows" link;
      do_stuff_here ()
    end
  (* If [link] is a directory, Lwt will enter it and try to
     delete its contents *)

A little grep in Lwt tells that only Lwt_io.with_temp_dir uses Lwt_unix.lstat.

@MisterDA MisterDA force-pushed the win32-lstat-is-not-stat branch 2 times, most recently from b62337b to f04feeb Compare September 16, 2021 09:10
Copy link
Collaborator

@avsm avsm left a comment

Choose a reason for hiding this comment

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

Yikes, this is quite a serious bug on Windows. @raphael-proust -- think we should cut a point release with this?

@smorimoto
Copy link
Member

Before cutting a point release, we have to resolve the incompatibility with 4.02 introduced by #880 or update the downward version constraint.

@MisterDA
Copy link
Contributor Author

An opam-grep for Lwt_unix.lstat gives the list of opam packages affected on Windows below. Some packages may be missing if they opened Lwt_unix.lstat. Thanks to @Ngoguey42 for running it.

[Info] Getting the list of all known opam packages..
[Info] Fetching and grepping using grep..
current matches your regexp.-----------------------------------------]  891/3500
current_ansi matches your regexp.------------------------------------]  892/3500
current_docker matches your regexp.----------------------------------]  893/3500
current_examples matches your regexp.--------------------------------]  894/3500
current_git matches your regexp.
current_github matches your regexp.----------------------------------]  896/3500
current_incr matches your regexp.------------------------------------]  897/3500
current_rpc matches your regexp.-------------------------------------]  899/3500
current_slack matches your regexp.-----------------------------------]  900/3500
current_web matches your regexp.
fat-filesystem matches your regexp.----------------------------------] 1144/3500
flow matches your regexp.--------------------------------------------] 1175/3500
lwt matches your regexp.##########-----------------------------------] 1678/3500
lwt_ppx matches your regexp.######-----------------------------------] 1683/3500
lwt_react matches your regexp.####-----------------------------------] 1684/3500
obuilder-spec matches your regexp.######-----------------------------] 2000/3500
obuilder matches your regexp.###########-----------------------------] 2001/3500
opam-monorepo matches your regexp.###########------------------------] 2231/3500
pvem_lwt_unix matches your regexp.###################----------------] 2664/3500
rashell matches your regexp.
release matches your regexp.##########################---------------] 2733/3500
tezt matches your regexp.######################################------] 3222/3500
:heavy_check_mark: [##################################################################] 3500/3500

@smorimoto smorimoto merged commit e12504e into ocsigen:master Oct 6, 2021
@MisterDA MisterDA deleted the win32-lstat-is-not-stat branch October 6, 2021 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants