891110 - [OS.File] Detect attempts to pass |undefined| to a C function

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

Passing |undefined| to a C function is always an error. We should detect this and throw an intelligible error.

This can be done easily by patching function |declareFFI| of osfile_shared_allthreads.jsm.

Comment 1

[Sahil Chelaramani]

Hi,
I'm a newbie. Does this mean that the variable arguments is undefined? and a error must be thrown on this condition? What else would need to be taken care of?

Comment 2

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

Hi Sahil,
If you take a look at |declareFFI|, you will see that it returns a function called |ffi|. This function takes a pseudo-array |arguments|. The objective, here, is to verify that none of the items in |arguments| is |undefined|, and to throw a readable |TypeError| if this is the case.

Comment 3

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

Hi Karim,
Indeed, there is no test for |declareFFI| itself, but there are many tests for functions declared using |declareFFI|. You can find these tests in worker_test_osfile_{unix, win}.js.

You mention that you already have a patch. Do you want me to assign the bug to you?

Comment 4

[Karim Hamidou]

Hi Yoric,

Ok, go ahead and assign the bug to me.

k

Comment 5

[Karim Hamidou]

Attachment: This is a tentative patch.

Actually, I'm not so sure about the unit test. Tell me what you think of it.

Comment 6

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

Comment on attachment 783271 [details] [diff] [review]
This is a tentative patch.

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

Thanks for the patch. Next time, if you want me to review it, please mark it as "review?", otherwise it won't show up on my dashboard.

Comment 7

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

Comment on attachment 783271 [details] [diff] [review]
This is a tentative patch.

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

I'd like a few changes mentioned below, but it's a good patch, thanks.
Also, you should change the commit message of your patch to remove "Basic patch", etc. and to add ";r=yoric" at the end.

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +887,5 @@
>      signature.push(current.implementation);
>    }
>    try {
>      let fun = lib.declare.apply(lib, signature);
>      let result = function ffi(/*arguments*/) {

Could you take the opportunity to upgrade this to JavaScript rest arguments instead of using the |arguments| keyword?
e.g. |function ffi(...args)| and use |args| instead of |arguments|.

@@ +889,5 @@
>    try {
>      let fun = lib.declare.apply(lib, signature);
>      let result = function ffi(/*arguments*/) {
> +      for(let i = 0; i < arguments.length; i++) {
> +        if(typeof arguments[i] == "undefined") {

Nit: we generally add a whitespace, e.g. "for (" instead of "for(", "if (" instead of "if(".

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js
@@ +36,5 @@
>    is(file, -1, "test_open_close: opening of non-existing file failed");
>    is(ctypes.errno, OS.Constants.libc.ENOENT, "test_open_close: error is ENOENT");
>  }
>  
> +function test_passing_undefined() 

Nit: Please remove the whitespace at the end of the line.

@@ +37,5 @@
>    is(ctypes.errno, OS.Constants.libc.ENOENT, "test_open_close: error is ENOENT");
>  }
>  
> +function test_passing_undefined() 
> +{

Nit: could you use the same indentation as the rest of this file?

@@ +47,5 @@
> +                                     OS.Constants.libc.S_IRWXU);
> +    } catch(e) {
> +        let te = new TypeError();
> +        is(e.constructor == te.constructor, "test_passing_undefined: exeption gets thrown")
> +    }

I believe that this would be more readable if you used
} catch (e if e instanceof TypeError) {
  // ...
}

Comment 8

[Karim Hamidou]

Attachment: bug-891110.patch

This time it should be correct.

Comment 9

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

Comment on attachment 785084 [details] [diff] [review]
bug-891110.patch

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

Patch is good. You have my r+, with a few trivial changes detailed below – assuming it passes tests.

Could you change the commit message to something along the lines of
Bug 891110 - Detecting attempts to pass undefined in declareFFI;r=yoric

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +889,5 @@
>    try {
>      let fun = lib.declare.apply(lib, signature);
> +    let result = function ffi(...args) {
> +      for (let i = 0; i < args.length; i++) {
> +        if(typeof args[i] == "undefined") {

Nit: Could you add a space between |if| and |(|?

@@ +890,5 @@
>      let fun = lib.declare.apply(lib, signature);
> +    let result = function ffi(...args) {
> +      for (let i = 0; i < args.length; i++) {
> +        if(typeof args[i] == "undefined") {
> +            throw new TypeError("Argument " + i + " of " + symbol + " is undefined");

Nit: It looks like your indentation is a bit larger than in the rest of this file. Could you fix this?

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js
@@ +49,5 @@
> +                                 OS.Constants.libc.S_IRWXU);
> +  } catch(e if e instanceof TypeError) {
> +    exceptionRaised = true;
> +  }
> +  

Nit: Could you remove that whitespace?

Comment 10

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

Automated tests: https://tbpl.mozilla.org/?tree=Try&rev=35854cbaa732

Comment 11

[Karim Hamidou]

Attachment: bug-891110-v2.patch

This time everything should be right.

Comment 12

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

Comment on attachment 786392 [details] [diff] [review]
bug-891110-v2.patch

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

Looks good to me. Thanks for the patch!

Comment 13

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

Do you know the procedure to land the patch?

Comment 14

[Karim Hamidou]

(In reply to David Rajchenbach Teller [:Yoric] from comment #13)
> Do you know the procedure to land the patch?

I guess I should add the "check-in needed" keyword to the page ? Apparently I don't have the rights to do it.

Comment 15

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

Ah, I keep forgetting who has right and who doesn't.
Landing patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Backed out for breaking almost all the test suites we have :|

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

TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - SyntaxError: 'arguments' object may not be used in conjunction with a rest parameter at resource://gre/modules/osfile/osfile_shared_allthreads.jsm:895
SyntaxError: 'arguments' object may not be used in conjunction with a rest parameter

(David, it seems like your try push is empty.)

Comment 18

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

Oh, my bad.
Karim, can you take a look at this?
It looks like you'll need to replace the outer |arguments| with a rest parameter, too.

Comment 19

[Karim Hamidou]

Hmm, okay. I've got a couple questions though :
- how come this wasn't caught by unit tests ?
- and how can I make sure that this doesn't happen again ?

Comment 20

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

This wasn't caught by my pushing to Try because it seems that I made a mistake while pushing.
If this wasn't caught by your tests, there is something fishy, too. Are you sure that you rebuilt your code properly before running your tests?

Comment 21

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

How are things faring? Do you need any assistance?

Comment 22

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

In the absence of news, de-assigning.

Comment 23

[Avinash [:hardfire]]

Attachment: 891110

I ran the test as `./mach mochitest-chrome toolkit/components/osfile/tests/mochi/test_osfile_front.xul` and everything worked fine :D ..
Please assign this bug to me too.

Comment 24

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

Comment on attachment 828635 [details] [diff] [review]
891110

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

Looks good to me.
Could you make the changed detailed below, to increase confidence in the test?

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js
@@ +192,5 @@
> +                                            | OS.Constants.libc.O_CREAT
> +                                            | OS.Constants.libc.O_TRUNC,
> +                                            OS.Constants.libc.S_IRWXU);
> +  } catch(e if e instanceof TypeError) {
> +    exceptionRaised = true;

Can you check that the message of |e| contains "open"? Just to be sure that we don't accidentally catch an unrelated TypeError.

Comment 25

[Avinash [:hardfire]]

Attachment: 891110-v2

Comment 26

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

Comment on attachment 828835 [details] [diff] [review]
891110-v2

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

Looks good to me.
Thanks for the patch :)

Comment 27

[Avinash [:hardfire]]

Attachment: 891110-v3

adding windows tests too, just changed the api function name, rest should remain same.

Comment 28

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

Comment on attachment 828846 [details] [diff] [review]
891110-v3

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_win.js
@@ +197,5 @@
> +  try {
> +    let file = OS.Win.File.open(undefined, OS.Constants.libc.O_RDWR
> +                                            | OS.Constants.libc.O_CREAT
> +                                            | OS.Constants.libc.O_TRUNC,
> +                                            OS.Constants.libc.S_IRWXU);

No, it's a Unix specific function.
For Windows, use OS.File.CreateFile, as detailed in http://msdn.microsoft.com/en-us/library/windows/desktop/aa363858%28v=vs.85%29.aspx

Comment 29

[Avinash [:hardfire]]

Attachment: 891110-v4

using OS.Win.File.CreateFile to test for windows.

Comment 30

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

Comment on attachment 828892 [details] [diff] [review]
891110-v4

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

Next step is testing it :)

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_win.js
@@ +202,5 @@
> +      null,
> +      OS.Constants.Win.OPEN_EXISTING,
> +      0,
> +      null);
> +  } catch(e if e instanceof TypeError && e.message.indexOf("open") > -1) {

Maybe "CreateFile" instead of "open".

Comment 31

[Avinash [:hardfire]]

Attachment: 891110-v5

yes! should be CreateFile . my mistake .. and now time to test it :D

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/0de129a3ddfc

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0de129a3ddfc