867406 - [OS.File] OS.File.move doesn't seem to handle failures

Status: RESOLVED INVALID

(Toolkit Graveyard :: OS.File, defect)

Description

[(no longer active)]

This code is sort of hard to read, but if I understand correctly, on Windows for example we just call MoveFileEx and hope for the best.  This is not safe at all since this call can and does fail in practice on Windows at least (and probably o other platforms as well.)  The effects of this extend beyond OS.File.move, for example, and writeAtomic API seems to call OS.File.move as well, so it's vulnerable to the same failure scenario, which, from the viewpoint of the caller of the API, would meant that writeAtomic would completely fail to write anything at all.

David, am I reading this code correctly?

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Well, if |MoveFileEx| returns 0, we throw an error. Am I missing something?

Comment 2

[(no longer active)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> Well, if |MoveFileEx| returns 0, we throw an error. Am I missing something?

Ah right, see the part about this code being hard to read.  :-)  I didn't know what throw_on_zero is.

But still, what does OS.file.writeAtomic do?  Just pass the exception through?  I'm not sure about the error handling semantics of this code.  From a quick look at the callers or writeAtomic, I can only see a couple which seem to have error handling logic around writeAtomic calls...

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> > Well, if |MoveFileEx| returns 0, we throw an error. Am I missing something?
> 
> Ah right, see the part about this code being hard to read.  :-)  I didn't
> know what throw_on_zero is.
> 
> But still, what does OS.file.writeAtomic do?  Just pass the exception
> through?

Yes, that's exactly what it does. The exception itself has the logic to translate (a number of) Windows error codes into something cross-platform.

>  I'm not sure about the error handling semantics of this code.
> From a quick look at the callers or writeAtomic, I can only see a couple
> which seem to have error handling logic around writeAtomic calls...

Comment 4

[(no longer active)]

OK then I think we should file individual bugs on all of the callers that don't do any error checking, and continue to watch for similar bugs in new callers... :/