Lwt: Lwt_io.open_file: Unix.O_TRUNC is a dangerous default for output channel

[StrykerKKD]

I think O_TRUNC is a little bit dangerous default, because the output channel modifies the file's content by just opening it. For me it's feels like an unexpected side effect. This behavior is also not documented, which made it harder to find out why my file kept getting truncated.

[aantron]

Agreed.

I'm not sure what the best way to address this is, though. O_TRUNC is pretty typical for many usages, and not supplying it by default can result in a surprising coeffect, files that are too large and contain unexpected (old) data. Perhaps this should only be clearly and loudly documented?

[StrykerKKD]

I think only the docs should be updated, because removing O_TRUNC cold be a big breaking change for a such a small "improvement".

[aantron]

Ok, docs included in the commit linked above. Hopefully, that is good enough to resolve this.

Thinking about it more, it seems best to keep O_TRUNC, not only because of breakage if we remove it. Without O_TRUNC, all existing and future instances of ordinary code like this:

Lwt_io.with_file ~mode:Output "foo" (fun channel ->
  Lwt_io.write ...)

...which are naively expected to leave foo in a predictable state, become incorrect. In the worst case, they become security vulnerabilities. I think O_TRUNC is the right default, otherwise we have an API that is too difficult to intuitively use safely, i.e. a broken API.

That said, it is still true that O_TRUNC is a surprising side effect when it is unwanted, so we do have to document it :)

[hcarty]

I ran into this recently. I wonder if it would be reasonable to make flags a required argument in a future, major release. That way the user is forced to decide which behavior they want.

[aantron]

I just searched packages that depend on Lwt in opam. At least the following packages would be affected by making this change: biocaml, cohttp, csv, csvprovider, dns-forward, flow, future, imap, imaplet-lwt, lambda-term, markup, mechaml, message-switch, mirror, plotkicadsch, pvem_lwt_unix.

I'm not certain if its worth doing, but I would like to do it.

[aantron]

If we only make ~flags required, the interface becomes a bit awkward. In

val open_file :
  ?buffer:Lwt_bytes.t ->
  flags:Unix.open_flag list ->
  ?perm:Unix.file_perm ->
  mode:'a mode ->
  file_name ->
    'a channel Lwt.t

the user would have to specify both ~flags:[O_RDONLY] and ~mode:Input, which seems redundant. Perhaps we should then have the ~mode argument add O_RDONLY to the list, if it or O_RDWR is not already present?

Another option might be to switch from using the Unix flags to a GADT, where the constructors O_RDONLY and O_WRONLY specify the phantom types input and output. Then, we can drop ~mode entirely. This would break even more users, though, as it would affect everyone.

[aantron]

We decided to close this again. A better API can be made than the existing openfile, but it's difficult to gradually change openfile into it:

• Making ~flags required breaks only some users, but the result is an awkward API, perhaps not worth the breakage.
• Changing openfile more thoroughly breaks all users.

So, it seems best to keep the flaws of openfile in mind if/when designing a replacement API, but leave the current openfile alone.

[hcarty]

From an separate discussion with @aantron - A useful longer term plan could be to create a Lwt equivalent of bos. Specifically the Bos.OS portion of the library.

That work could be done separately from core Lwt. If there's demand the code could be proposed for inclusion in Lwt itself.

[raphael-proust]

@hcarty feedback on this early prototype is welcome: https://gitlab.com/raphael-proust/boloss
(Although this thread is maybe not the most appropriate place to discuss this library, I put the call here so it can be found easily.)