1704495 - Verify mappings for NS_ERROR_* errors in nsLocalFileWin.cpp

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Jens Stutte [:jstutte]]

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?

Comment 1

[Jens Stutte [:jstutte]]

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.

Comment 2

[Jens Stutte [:jstutte]]

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_INVALID_HANDLE	NS_ERROR_INVALID_HANDLE (new)	Self explaining.
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."

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 4

[Jens Stutte [:jstutte]]

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

Comment 5

[Jens Stutte [:jstutte]]

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.

Comment 6

[Jan Rio Krause]

I will take care of it.

Comment 7

[Jan Rio Krause]

Attachment: Bug 1704495 - Map Windows error `ERROR_NOT_READY` to `NS_ERROR_FILE_DEVICE_TEMPORARY_FAILURE`. r=#xpcom-reviewers,#dom-storage-reviewers

Comment 8

[Jan Rio Krause]

Attachment: Bug 1704495 - Map Windows errors `ERROR_INVALID_BLOCK`, `ERROR_INVALID_HANDLE`, `ERROR_ARENA_TRASHED` to `NS_ERROR_FILE_INVALID_HANDLE`. r=#xpcom-reviewers,#dom-storage-reviewers

Depends on D130905

Comment 9

[Jan Rio Krause]

Attachment: Bug 1704495 - Map Windows error `ERROR_CANNOT_MAKE` to `NS_ERROR_FILE_ACCESS_DENIED`. r=#xpcom-reviewers,#dom-storage-reviewers

Depends on D130906

Comment 10

[Jan Rio Krause]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/faab6442b9d2
Map Windows error `ERROR_NOT_READY` to `NS_ERROR_FILE_DEVICE_TEMPORARY_FAILURE`. r=xpcom-reviewers,dom-storage-reviewers,nika,jstutte
https://hg.mozilla.org/integration/autoland/rev/a9410a641017
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
https://hg.mozilla.org/integration/autoland/rev/24e488b549be
Map Windows error `ERROR_CANNOT_MAKE` to `NS_ERROR_FILE_ACCESS_DENIED`. r=xpcom-reviewers,dom-storage-reviewers,nika,jstutte

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/faab6442b9d2
https://hg.mozilla.org/mozilla-central/rev/a9410a641017
https://hg.mozilla.org/mozilla-central/rev/24e488b549be