If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED INVALID

Status

()

Toolkit
OS.File
RESOLVED INVALID
4 years ago
4 years ago

People

(Reporter: Ehsan, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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?
Well, if |MoveFileEx| returns 0, we throw an error. Am I missing something?
Flags: needinfo?(ehsan)
Summary: OS.File.move doesn't seem to handle failures → [OS.File] OS.File.move doesn't seem to handle failures
(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...
Flags: needinfo?(ehsan)
(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...
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... :/
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.