[PATCH] Add ability for can_write to indicate why the file is not writable

[Richard Smith]

On Thu, Sep 4, 2014 at 10:47 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, Sep 4, 2014 at 4:49 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>
>> On Wed, Sep 3, 2014 at 9:36 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > Hi dblaikie,
>> >
>> > Add an optional output parameter to `can_write` to allow it to indicate
>> why a file is not considered writable (along with a sample use from Clang
>> where we currently produce stupid diagnostics saying `"can't open output
>> file: Success"`).
>> >
>> > I'm not entirely sure this is the right design, and in particular it's
>> a somewhat different interface from the other functions in this file. I
>> also considered:
>> >
>> >   std::error_code can_write(const Twine &Path, bool &result);
>> >
>> > ... which seems redundant because `result` is set to `true` iff the
>> error code is `success`, and ...
>> >
>> >   std::error_code can_write(const Twine &Path);
>> >
>> > ... which is terrible because `bool(can_write(Path))` is `true` iff the
>> file is *not* writable, and ...
>> >
>> >   std::error_code cannot_write(const Twine &Path);
>> >
>> > ... which is inconsistent with other functions near here and ugly.
>>
>> I don't think this API (in general) is a good one to have in the first
>> place as it suffers from TOCTOU issues (as do our uses of other APIs
>> like fs::exists). A better approach is for places like
>> CompilerInstance.cpp is to simply open the file for writing and report
>> the various ways that can fail instead of asking questions for which
>> 1) the answer can change subsequent to asking the question anyway, and
>> 2) the answer isn't canonical (there's a lot more reasons for not
>> being able to write to a file than its "read-only" attribute being set
>> on Windows, for instance).
>>
>
> +1 to all of that. Chandler & others have tales of TOCTOU stuff that
> scares me sufficiently to assume that's all applicable & worth worrying
> about, but perhaps Richard has a notion as to why it might not apply here?
> Otherwise I'm all for making this a "try to do the thing you want (open for
> write, etc) and see if it fails" rather than querying that property
> separately.
>

There's no TOCTOU issue here. We're merely doing an early check to see if a
later operation is likely to fail so we can save some work and exit
earlier. We can't "try to do the thing we want" because the thing we want
is a rename, and we've not built the source file yet.

The code is approximately:

  if (exists(dest) && !can_write(dest)) {
    fill in error code;
    return;
  }

  produce_temporary_file();
  if (!copy_temporary_file_to_dest()) {
    fill in error code;
    return;
  }

... and the problem is that we can't fill in the error code in the first
case.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140904/ecbf7ac6/attachment.html>