952997 - OS.File.getPosition/.setPosition broken (for large files) on Windows

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Nils Maier [:nmaier]]

OS.File.getPosition/.setPosition are totally broken for large files (position > MAXINT32)

1) .getPosition will just use the return value of .setPosition. The latter is broken, hence the former is broken as well.

2) The Windows backend uses SetFilePointer, which returns a DWORD.
2.1) The backend incorrectly defines DWORD as int32_t, but a DWORD is actually unsigned.
2.2) The backend checks the return value of SetFilePointer to be negative, which is bad due to 2.1 coercing good uint32 values to bad int32 ones. While the incorrect cast is bad enough, it also disregards the SetFilePointer API, which specifies that "If the function returns a value other than INVALID_SET_FILE_POINTER, the call to SetFilePointer has succeeded."

3) The Windows backend does not support 64-bit seeks at all.
3.1) The SetFilePointer API has a mechanism whereby 64-bit values should be provided using two longs (low and high), whereas high is optional for int32 seeks.
3.2) The Windows backend does not even attempt to use the high long for 64-bit seeks and tries to coerce the position to int32, which will of course fail. (Well, at least it actually generates an error).

4) Due to 2.) it is not possible to split 64-bit seeks into multiple int32 seeks to work around 3.), by calling the function repeatedly. SetFilePointer will return a DWORD with the current position in this case. As 2.) coerces that uint32 to int32 and then checks for a negative value as the error condition, it will fail if the coercion generates a negative number, which is a given when splitting the seek.
4.1) You can work around this by catching the exception and ignoring it if .winLastError is 0 (aka success).
4.2) Because of this, getPosition is broken when the current position is > MAXINT32, and .setPosition is broken if the new position is > MAXINT32, at least when using a compiler that does these kinds of overflows, e.g. MSVC/cl in the used configuration. Other compilers might just trap that and cause a full abort() or do something entirely stupid, so the current behavior and the work around from 4.1.) relies on undefined behavior.

There are no workarounds for .getPosition except tracking the position yourself and avoiding that API altogether.

I also recognize that there is no int64 type in Javascript, only IEEE doubles. However those doubles alone can be used to hold integers up to 2^53 without loss of precision IIRC. (And also, there is always the inconvenient int64 ctype).
So I'd expect the API to be at least capable of doing "uint53" seeks right, which, as it turns out, seems to be the case when it comes to the Unix backend (so this bug is Windows only).

Comment 1

[Nils Maier [:nmaier]]

Attachment: Fix OS.File large file support.

Well, defining all DWORD as unsigned would have been very invasive, as js doesn't know uints, and therefore ORing flags together would have been problematic and would have also required changes in OS.Constants, so I opted for a real DWORD/DWORD_FLAGS split instead.

The rest is pretty much straight forward:
- Make (int64) projections fail if a value cannot be represented in JS.
- Fix setPosition on Windows. getPosition is for free.
- Add some tests. Should be noted that the tests do not actually create large files, just seek around without writing data. Unless an OS is horribly broken, the test files should stay at 0 bytes.

Comment 2

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

Comment on attachment 8356545 [details] [diff] [review]
Fix OS.File large file support.

Review of attachment 8356545 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the catch.
r=me with a few minor changes

::: toolkit/components/osfile/modules/osfile_win_back.jsm
@@ +103,5 @@
>         /**
> +        * Not actually a DWORD, as it is signed. But it makes OS.Constants
> +        * easier to deal with, in particular because bit operators in JS coerce
> +        * numbers to int32_t, which would make it a pain to compose uint32_t
> +        * flags.

I understand your manipulation, but your documentation is too obscure.

Maybe something along the lines of: « A special type used to represent flags passed as a DWORD to a function.

In JavaScript, bitwise manipulation of numbers, such as or-ing flags, can produce negative numbers. Since DWORD is signed, these negative numbers simply cannot be converted to DWORD. For this reason, whenever bit manipulation is called for, you should rather use DWORD_FLAGS, which is represented as a signed integer, hence has the correct semantics. »

@@ +316,5 @@
>                      /*file*/   Type.HANDLE);
>  
>         declareLazyFFI(SysFile, "SetFilePointer", libc,
>           "SetFilePointer", ctypes.winapi_abi,
> +                    /*return*/ Type.DWORD,

Ah, good catch.

::: toolkit/components/osfile/modules/osfile_win_front.jsm
@@ +18,5 @@
>  
>    (function(exports) {
>       "use strict";
>  
> +     const INVALID_SET_FILE_POINTER = 0xffffffff; // (DWORD)-1

0x notation is deprecated in JavaScript.
Why don't you use OS.Constants.Win.INVALID_SET_FILE_POINTER?

Comment 3

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #2)
> ::: toolkit/components/osfile/modules/osfile_win_front.jsm
> @@ +18,5 @@
> >  
> >    (function(exports) {
> >       "use strict";
> >  
> > +     const INVALID_SET_FILE_POINTER = 0xffffffff; // (DWORD)-1
> 
> 0x notation is deprecated in JavaScript.

Are you sure? This is news to me. Octal notation is deprecated (0num), and 0o octal representation was just introduced, which would make it rather curious if 0x was deprecated.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-literals-numeric-literals
and 0num (LegacyOctalIntegerLiteral) deprecation:
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-additional-syntax-numeric-literals

> Why don't you use OS.Constants.Win.INVALID_SET_FILE_POINTER?

Good point ;)

Comment 4

[Nils Maier [:nmaier]]

> > Why don't you use OS.Constants.Win.INVALID_SET_FILE_POINTER?
> 
> Good point ;)

I remember now:
0xffffffff === 4294967295 while OS.Constants.Win.INVALID_SET_FILE_POINTER === -1

So it must be either:
(result == INVALID_SET_FILE_POINTER) (0xffffffff)
or:
((result | 0) == OS.Constants.Win.INVALID_SET_FILE_POINTER) (-1)
or:
Changing OSFileConstants to make INVALID_SET_FILE_POINTER 0xffffffff instead of -1 (using a double instead of an int in C++)

Which one do you prefer?

Comment 5

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

I'd rather we fix OS.Constants.Win.INVALID_SET_FILE_POINTER, if it is incorrect.

Comment 6

[Nils Maier [:nmaier]]

Attachment: Fix OS.File large file support.

Alright, I changed OS.Constants.Win.INVALID_SET_FILE_POINTER and also fixed the mochitest for the backend to use uint/DWORD where needed.

Part of this green(-ish) try:
https://tbpl.mozilla.org/?tree=Try&rev=793fac2824c2

Comment 7

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

Comment on attachment 8357288 [details] [diff] [review]
Fix OS.File large file support.

Review of attachment 8357288 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

Side-note: in the future, could you add version numbers to your patch titles when uploading them? It simplifies interdiffing.

::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_largefiles.js
@@ +108,5 @@
> +        yield file.setPosition(1, OS.File.POS_CURRENT);
> +        do_throw("Shouldn't have succeeded");
> +      } catch (ex) {
> +        do_print(ex.toString());
> +        do_check_true(!!ex.message);

I don't understand that change.

Comment 8

[Nils Maier [:nmaier]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #7)
> ::: toolkit/components/osfile/tests/xpcshell/test_osfile_async_largefiles.js
> @@ +108,5 @@
> > +        yield file.setPosition(1, OS.File.POS_CURRENT);
> > +        do_throw("Shouldn't have succeeded");
> > +      } catch (ex) {
> > +        do_print(ex.toString());
> > +        do_check_true(!!ex.message);
> 
> I don't understand that change.

Sorry, I wanted to mention this, but forgot about it in the time it took my try to run.
Turns out some lseek or file systems implementations (Linux) will outright reject large seeks beyond 53-bit. So they will bail with something like Operation not permitted, etc. The previous check was therefore too specific and wouldn't have passed. What we are really interested in is that OS.File throws an error, because either the underlying lseek failed or the resulting file pointer cannot be projected to a double.
I left the do_check_true in because it still verifies the error has a non-empty message, and because it verifies in the test logs that the code path was actually taken should a problem arise in the future.

Comment 9

[Nils Maier [:nmaier]]

Let's get it landed.

Part of this green(-ish) try:
https://tbpl.mozilla.org/?tree=Try&rev=793fac2824c2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/13bea55207f2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures. Please make sure this passes on Try before requesting checkin again.
https://hg.mozilla.org/integration/fx-team/rev/8c9bf1bb1c4e

https://tbpl.mozilla.org/php/getParsedLog.php?id=32824376&tree=Fx-Team

Comment 12

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

Any news, Nils?

Comment 13

[Nils Maier [:nmaier]]

Yeah, sorry. I didn't really have time these past weeks, but I'll probably get around to fix this (the test) next week...

Comment 14

[Nils Maier [:nmaier]]

Attachment: Fix OS.File large file support.

Well, that took a couple of more weeks longer to get to than I anticipated.

Anyway, here is an unbitrotted version that also fixes the previous test failure.

Green try: https://tbpl.mozilla.org/?tree=Try&rev=6e06f7a58e46

Comment 15

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

Comment on attachment 8403181 [details] [diff] [review]
Fix OS.File large file support.

Review of attachment 8403181 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Thank you very much.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/7a706b3dd3c9

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7a706b3dd3c9