Permissions and deletion of temporary files created by preprocessors (pp)

[jboillot]

Hello,
When a preprocessor (pp) is used, the input file is passed as an argument to the preprocessor executable and then redirected to a temporary file before being read again and deleted.

merlin/src/ocaml/driver/pparse.ml

Line 162 in ce92495

!System.run_in_directory

The first problem I have is that, when correctly read, the temporary file is never deleted which leads to an accumulation of merlinpp****.out in the /tmp/ directory. I propose a simple fix of this issue in PR #1801.

The second problem is that the temporary file is created, at least on Linux, via a redirection of the stdout of the preprocessor program to a temporary file.

merlin/src/utils/std.ml

Line 787 in ce92495

Printf.sprintf "%s 1>%s" args stdout

So, the permissions of this file are by default, at least on my computer, 0o644 which means everybody can read its content. Note that in the case of the input file, it is copied via a call to the function Filename.temp_file that uses permissions 0o600 (only owner can read and write).
I know this file only exists briefly (at least after the patch), and reading the source files requires being able to execute commands, but I'm not comfortable having source files accessible to everyone.
A possible fix could be to first create the source file via Filename.temp_file and then write into it with the redirection? I think this is the only place where the function System.run_in_directory is used with the stdout argument set. Should I include this in the PR? Should the creation of such a file be done in System.run_in_directory or in pparse.ml?

Have a good day!

[voodoos]

Hi @jboillot, thanks for the report and the PR !

I will try to have a look at it soon. I am curious about the interaction with Merlin caching mechanism, did you check (I did not, yet) that Merlin was not relying on re-reading this file in the absence of changes ?

Also, is that really an issue, shouldn't the system clean the tmp directory regularly ?

Seems right to make the permissions narrower in any case :-)