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

RESOLVED FIXED in mozilla28

Status

()

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

People

(Reporter: Yoric, Assigned: hardfire)

Tracking

(Blocks: 1 bug)

unspecified
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=js][Async][engineering-friend])

Attachments

(1 attachment, 7 obsolete attachments)

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.
Depends on: 891300

Comment 1

4 years ago
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?
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.
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?
Flags: needinfo?(karim)

Comment 4

4 years ago
Hi Yoric,

Ok, go ahead and assign the bug to me.

k
Flags: needinfo?(karim)

Comment 5

4 years ago
Created attachment 783271 [details] [diff] [review]
This is a tentative patch.

Actually, I'm not so sure about the unit test. Tell me what you think of it.
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.
Attachment #783271 - Flags: review?(dteller)
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) {
  // ...
}
Attachment #783271 - Flags: review?(dteller) → feedback+

Comment 8

4 years ago
Created attachment 785084 [details] [diff] [review]
bug-891110.patch

This time it should be correct.
Attachment #783271 - Attachment is obsolete: true
Attachment #785084 - Flags: review?(dteller)
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?
Attachment #785084 - Flags: review?(dteller) → review+
Automated tests: https://tbpl.mozilla.org/?tree=Try&rev=35854cbaa732

Comment 11

4 years ago
Created attachment 786392 [details] [diff] [review]
bug-891110-v2.patch

This time everything should be right.
Attachment #785084 - Attachment is obsolete: true
Attachment #786392 - Flags: review?(dteller)
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!
Attachment #786392 - Flags: review?(dteller) → review+
Do you know the procedure to land the patch?
Flags: needinfo?(nobody)

Comment 14

4 years ago
(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.
Ah, I keep forgetting who has right and who doesn't.
Landing patch.
Flags: needinfo?(nobody)
Keywords: checkin-needed

Updated

4 years ago
Whiteboard: [mentor=Yoric][lang=js][Async] → [mentor=Yoric][lang=js][Async][engineering-friend]

Updated

4 years ago
Assignee: nobody → karim
https://hg.mozilla.org/integration/fx-team/rev/ef8248e892e0
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js][Async][engineering-friend] → [mentor=Yoric][lang=js][Async][engineering-friend][fixed-in-fx-team]
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.)
Whiteboard: [mentor=Yoric][lang=js][Async][engineering-friend][fixed-in-fx-team] → [mentor=Yoric][lang=js][Async][engineering-friend]
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

4 years ago
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 ?
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?
How are things faring? Do you need any assistance?
Flags: needinfo?(karim)
In the absence of news, de-assigning.
Flags: needinfo?(karim)
(Assignee)

Comment 23

4 years ago
Created attachment 828635 [details] [diff] [review]
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.
Attachment #828635 - Flags: review?(dteller)
Assignee: karim → avinash
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.
Attachment #828635 - Flags: review?(dteller) → review+
(Assignee)

Comment 25

4 years ago
Created attachment 828835 [details] [diff] [review]
891110-v2
Attachment #828635 - Attachment is obsolete: true
Attachment #828835 - Flags: review?(dteller)
Comment on attachment 828835 [details] [diff] [review]
891110-v2

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

Looks good to me.
Thanks for the patch :)
Attachment #828835 - Flags: review?(dteller) → review+
(Assignee)

Comment 27

4 years ago
Created attachment 828846 [details] [diff] [review]
891110-v3

adding windows tests too, just changed the api function name, rest should remain same.
Attachment #828835 - Attachment is obsolete: true
Attachment #828846 - Flags: review?(dteller)
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
Attachment #828846 - Flags: review?(dteller)
(Assignee)

Comment 29

4 years ago
Created attachment 828892 [details] [diff] [review]
891110-v4

using OS.Win.File.CreateFile to test for windows.
Attachment #828846 - Attachment is obsolete: true
Attachment #828892 - Flags: review?(dteller)
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".
Attachment #828892 - Flags: review?(dteller) → review+
(Assignee)

Comment 31

4 years ago
Created attachment 828929 [details] [diff] [review]
891110-v5

yes! should be CreateFile . my mistake .. and now time to test it :D
Attachment #828892 - Attachment is obsolete: true
Attachment #828929 - Flags: review?(dteller)
Attachment #828929 - Flags: review?(dteller) → review+
Attachment #786392 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/0de129a3ddfc
Keywords: checkin-needed
Whiteboard: [mentor=Yoric][lang=js][Async][engineering-friend] → [mentor=Yoric][lang=js][Async][engineering-friend][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0de129a3ddfc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=Yoric][lang=js][Async][engineering-friend][fixed-in-fx-team] → [mentor=Yoric][lang=js][Async][engineering-friend]
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.