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 setting Lwt_process env on Windows #967

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mroch
Copy link

@mroch mroch commented Oct 19, 2022

env is an "environment block": a null-terminated block of null-terminated strings. using caml_stat_strdup_to_os on it doesn't work, it only copies the first string.

instead, we need to use an explicit length (caml_string_length(env)), but there's no public *_to_os function for this so we have to use the internal caml_win32_multi_byte_to_wide_char directly.

Fixes #966

@raphael-proust
Copy link
Collaborator

Thanks a lot for the MR. I don't have a windows machine to test this on so I can't reproduce locally. Nevertheless, I'll be reviewing this MR soon.

@mroch
Copy link
Author

mroch commented Oct 20, 2022

i dunno if you can get CI to run on the first diff (1d199cd) but it'll show the bug. for example: https://github.com/mroch/lwt/actions/runs/3284275234/jobs/5410034604

I believe the EINVAL comes from passing just a single null terminated string, without the additional \0 to terminate the block (and it's only passing the first of many envs, but I think it's the malformed block that causes the EINVAL specifically)

@raphael-proust
Copy link
Collaborator

i dunno if you can get CI to run on the first diff

I don't know either. I don't understand github's UI very well. ❓ ❗

But that's ok. When I do the review I might push a branch with just the first commit to force the CI.

Copy link
Contributor

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

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

Thanks for finding this and fixing the bug. The code is semantically identical to OCaml's Unix library, so I think this is good to go.
https://github.com/ocaml/ocaml/blob/c6d207656de19d97b5edf5c2f9028412aa218571/otherlibs/unix/createprocess.c#L120-L131

@raphael-proust
Copy link
Collaborator

At first I was pleased that the code was so similar.
Then I thought it'd be even better if the code was even more similar.
And then I started thinking about licenses and now I don't know what to do about this PR.

I'll ask if it's ok. Basically, the added code is calling the functions provided by the ocaml project in the appropriate sequence. I think the alternative to using the same code is making a pull-request on the ocaml project to provide this sequence of calls as a function itself to be called. That might not be desirable upstream anyway…

Anyway, I'll ask.

@raphael-proust
Copy link
Collaborator

A similar situation happened on the eio bugtracker:
ocaml-multicore/eio#352 (comment)

It's not exactly the same (not the same code being copied, not the same size of code) but it's also copying parts of the C code.

@MisterDA MisterDA mentioned this pull request Aug 4, 2023
@raphael-proust
Copy link
Collaborator

I've made raphael-proust/ocaml@bdbddd2 which aims to expose the code that we actually need from the OCaml source. Please leave comments on there, I don't know that I did anything correctly.

@raphael-proust
Copy link
Collaborator

As per ocaml/ocaml#12492 (comment) it turns out licensing is not an issue. We can go ahead on this PR.

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.

passing env in Lwt_process doesn't work on Windows
3 participants