Closed Bug 1704495 Opened 3 years ago Closed 2 years ago

Verify mappings for NS_ERROR_* errors in nsLocalFileWin.cpp


(Core :: XPCOM, task)




96 Branch
Tracking Status
firefox96 --- fixed


(Reporter: jstutte, Assigned:




(3 files)

ConvertWinError makes some mappings that might be unexpected. Examples (non exhaustive and TBD):

Windows error NS_ERROR_*
ERROR_NOT_READY NS_ERROR_FILE_NOT_FOUND "The device is not ready." requires other mitigations/user actions than ERROR_FILE_NOT_FOUND.
ERROR_NOT_SAME_DEVICE NS_ERROR_FILE_ACCESS_DENIED This error should occur only during file move of hard linked files between devices. The relation to denied access is not clear.
ERROR_INVALID_BLOCK NS_ERROR_OUT_OF_MEMORY "The storage control block address is invalid." seems not very related to memory.
ERROR_INVALID_HANDLE NS_ERROR_OUT_OF_MEMORY I would expect this to be handled by the abstraction layer and never be exposed as an error. Thus if it still occurs it would be something like NS_ERROR_INTERNAL_ERROR, IMHO
ERROR_ARENA_TRASHED NS_ERROR_OUT_OF_MEMORY Also this looks more like a UAF of something inside the abstraction layer and thus an NS_ERROR_INTERNAL_ERROR?
No longer depends on: 1690326
See Also: → 1690326
See Also: → 1705013
Windows error NS_ERROR_*
ERROR_CANNOT_MAKE NS_ERROR_FILE_ALREADY_EXISTS "The directory or file cannot be created." does not seem related to an already existing file, that has its own error. And mingw maps it to EACCES, thus NS_ERROR_FILE_ACCESS_DENIED seems more appropriate.

I'd propose the following mappings:

Windows error NS_ERROR_*
ERROR_NOT_READY NS_ERROR_FILE_DEVICE_TEMPORARY _FAILURE (new) "The device is not ready." is hopefully a temporary condition and we might want to mitigate this with retrys.
ERROR_NOT_SAME_DEVICE NS_ERROR_FILE_NOT_FOUND (exists) This error should occur only during file move of hard linked files between devices. In the end the file does not exist for us. Caveat: We might fail if we want to create it afterwards with the same filename.
ERROR_INVALID_BLOCK NS_ERROR_INVALID_HANDLE (new) "The storage control block address is invalid." means probably we are trying to use an old handle.
ERROR_ARENA_TRASHED NS_ERROR_INVALID_HANDLE (new) Also this looks more like a UAF of something, though I am not 100% sure what can cause this.
ERROR_CANNOT_MAKE NS_ERROR_FILE_ACCESS_DENIED (exists) "The directory or file cannot be created."

The leave-open keyword is there and there is no activity for 6 months.
:nika, maybe it's time to close this bug?

Flags: needinfo?(nika)

Not sure why we had the leave-open flag here - we did not land a single patch, yet.
Jan-Rio, given your current work on other error mappings, might you want to take a look also here? Thanks

Flags: needinfo?(nika) → needinfo?(jkrause)
Keywords: leave-open

The work here should probably include a check, if and how eventual callers reacted on the old error values and if we should thus make them aware of the new ones. And the mappings are just a proposal and might need further reasoning.

I will take care of it.

Assignee: nobody → jkrause
Flags: needinfo?(jkrause)

After discussing with Jens, we decided to not change ERROR_NOT_SAME_DEVICE at the moment. The documentation says "The system cannot move the file to a different disk drive.", so the proposal to map it to NS_ERROR_FILE_NOT_FOUND (see comment 2) does not fit perfectly.

Pushed by
Map Windows error `ERROR_NOT_READY` to `NS_ERROR_FILE_DEVICE_TEMPORARY_FAILURE`. r=xpcom-reviewers,dom-storage-reviewers,nika,jstutte
Map Windows errors `ERROR_INVALID_BLOCK`, `ERROR_INVALID_HANDLE`, `ERROR_ARENA_TRASHED` to `NS_ERROR_FILE_INVALID_HANDLE`. r=xpcom-reviewers,dom-storage-reviewers,nika,jstutte
Map Windows error `ERROR_CANNOT_MAKE` to `NS_ERROR_FILE_ACCESS_DENIED`. r=xpcom-reviewers,dom-storage-reviewers,nika,jstutte
You need to log in before you can comment on or make changes to this bug.