973538 - Harden OS.Path.join() and OS.Path.winGetDrive()

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[:gerard-majax]

In bug 961745 we discovered that the precompilation cache step that happens during |make package| might fail because.

This is because the precompilation step does an import on JS files. With DownloadAPI.jsm, we have some code path that gets triggered and does a OS.Path.join(Constants.Path.profileDir, ...). However, during this specific step, Constants.Path.profileDir is undefined and we are okay with this.

But the OS.Path.join() code does not check this and fails because of a TypeError when trying to access the 'length' property of this undefined object, at http://hg.mozilla.org/mozilla-central/annotate/2bddbd180d2d/toolkit/components/osfile/modules/ospath_unix.jsm#l86

OS.Path.winGetDrive() exposes a similar behavior.

We propose to gracefully throw an exception in both methods when we have some undefined path components. This way, the try/catch mechanisms that lives in the precompilation cache code path should correctly handle the case.

Comment 1

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

Note that we need to maintain the semantics of throwing an error. As I understand gerard-majax' suggestion, we just want to throw a well-defined error instead of a random TypeError.

Comment 2

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Please find attached a first patch. This patch does the job, but it seems to expose a nasty race condition that results in a segfault.

Comment 3

[:gerard-majax]

I tested a fix for the race in bug 973637. A Try build is pending for this bug with fix from bug 973637 applied: https://tbpl.mozilla.org/?tree=Try&rev=a7c14c769c45.

Comment 4

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #3)
> I tested a fix for the race in bug 973637. A Try build is pending for this
> bug with fix from bug 973637 applied:
> https://tbpl.mozilla.org/?tree=Try&rev=a7c14c769c45.

Try was green. Bug 973637 is about to get fixed.

Comment 5

[:gerard-majax]

Comment on attachment 8377142 [details] [diff] [review]
0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Time to ask review, since bug 973637 will be fixed soon :)

Comment 6

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

Comment on attachment 8377142 [details] [diff] [review]
0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

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

::: toolkit/components/osfile/modules/ospath_unix.jsm
@@ +83,3 @@
>    // If there is a path that starts with a "/", eliminate everything before
>    let paths = [];
>    for each(let i in arguments) {

Could you take the opportunity to make this function use rest arguments instead of |arguments|?

@@ +83,5 @@
>    // If there is a path that starts with a "/", eliminate everything before
>    let paths = [];
>    for each(let i in arguments) {
> +    if (!i) {
> +      throw new Error("undefined value in arguments");

"undefined path component", rather.
Also, make it a TypeError.

Same for Windows.

::: toolkit/components/osfile/modules/ospath_win.jsm
@@ +182,5 @@
>   * includes "\\\\").
>   */
>  let winGetDrive = function(path) {
> +  if (!path) {
> +    throw new Error("path is undefined");

TypeError

Comment 7

[:gerard-majax]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #6)
> Comment on attachment 8377142 [details] [diff] [review]
> 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch
> 
> Review of attachment 8377142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/modules/ospath_unix.jsm
> @@ +83,3 @@
> >    // If there is a path that starts with a "/", eliminate everything before
> >    let paths = [];
> >    for each(let i in arguments) {
> 
> Could you take the opportunity to make this function use rest arguments
> instead of |arguments|?

I'm not sure to get your point there :(

> 
> @@ +83,5 @@
> >    // If there is a path that starts with a "/", eliminate everything before
> >    let paths = [];
> >    for each(let i in arguments) {
> > +    if (!i) {
> > +      throw new Error("undefined value in arguments");
> 
> "undefined path component", rather.
> Also, make it a TypeError.
> 
> Same for Windows.

Done.

> 
> ::: toolkit/components/osfile/modules/ospath_win.jsm
> @@ +182,5 @@
> >   * includes "\\\\").
> >   */
> >  let winGetDrive = function(path) {
> > +  if (!path) {
> > +    throw new Error("path is undefined");
> 
> TypeError

Done.

Comment 8

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #7)
> (In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from
> comment #6)
> > Comment on attachment 8377142 [details] [diff] [review]
> > 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch
> > 
> > Review of attachment 8377142 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/osfile/modules/ospath_unix.jsm
> > @@ +83,3 @@
> > >    // If there is a path that starts with a "/", eliminate everything before
> > >    let paths = [];
> > >    for each(let i in arguments) {
> > 
> > Could you take the opportunity to make this function use rest arguments
> > instead of |arguments|?
> 
> I'm not sure to get your point there :(

Done.

Comment 9

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Updating: should address all previous remarks (TypeError, using rest arguments)

Comment 10

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

Comment on attachment 8381423 [details] [diff] [review]
0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

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

Looks good, thanks.
Note: switching from |arguments| to rest arguments was just a way to make the code more readable.

::: toolkit/components/osfile/modules/ospath_unix.jsm
@@ +83,3 @@
>    // If there is a path that starts with a "/", eliminate everything before
>    let paths = [];
> +  for each(let subpath in path) {

Nit: whitespace after |each|.

Comment 11

[:gerard-majax]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #10)
> Comment on attachment 8381423 [details] [diff] [review]
> 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch
> 
> Review of attachment 8381423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thanks.
> Note: switching from |arguments| to rest arguments was just a way to make
> the code more readable.
> 
> ::: toolkit/components/osfile/modules/ospath_unix.jsm
> @@ +83,3 @@
> >    // If there is a path that starts with a "/", eliminate everything before
> >    let paths = [];
> > +  for each(let subpath in path) {
> 
> Nit: whitespace after |each|.

Done. I'm waiting on Try to make sure we break nothing.

Comment 12

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Addressing latest nits, keeping r+.

Comment 13

[:gerard-majax]

Looks like some code is breaking: https://tbpl.mozilla.org/php/getParsedLog.php?id=35234151&tree=Try#error1

David, I see some 'makeDir' messages with some payload that may contain null values. Should we address this in this bug, open a new bug and fix it before landing this one?

Comment 14

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

Might as well do it now.

Comment 15

[:gerard-majax]

We get into a join("/some/correct/path", ""). I suspect we want to handle this gracefully and not throw in this case.

Comment 16

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Updated patch. Maybe we do not want to do the paths.push(subpath) if subpath.length == 0 ?

Comment 17

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

Comment on attachment 8381772 [details] [diff] [review]
0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

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

Looks good, with a few trivial changes.

::: toolkit/components/osfile/modules/ospath_unix.jsm
@@ +83,4 @@
>    // If there is a path that starts with a "/", eliminate everything before
>    let paths = [];
> +  for each (let subpath in path) {
> +    if (!subpath && subpath != "") {

Actually,
 if (!subpath) {
 }
covers both cases.

@@ +83,5 @@
>    // If there is a path that starts with a "/", eliminate everything before
>    let paths = [];
> +  for each (let subpath in path) {
> +    if (!subpath && subpath != "") {
> +      throw new TypeError("undefined path component");

Turns out it's "undefined or empty".

@@ +85,5 @@
> +  for each (let subpath in path) {
> +    if (!subpath && subpath != "") {
> +      throw new TypeError("undefined path component");
> +    }
> +    if (subpath.length != 0 && subpath[0] == "/") {

Since we have already checked that |subpath| is not falsy, we can get rid of the |subpath.length| check.

::: toolkit/components/osfile/modules/ospath_win.jsm
@@ +141,4 @@
>    let root;
>    let absolute = false;
>    for each(let subpath in path) {
> +    if (!subpath && subpath != "") {

As for Unix.

Comment 18

[:gerard-majax]

I'll do the change, but I'm not sure to get your point. Some tests are failing because they are building a path with some components that are valid bug empty strings. From your first and second remarks, I get that you want to make them still invalid. Is this really what we want ?

OS.Path.join("a", "b", "c", ""); will throw ?

Btw Try still fails those tests on windows.

Comment 19

[:gerard-majax]

Try is still failing on windows because of the same kind of behavior for winGetDrive("directory", "").

Comment 20

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

(In reply to Alexandre LISSY :gerard-majax from comment #18)
> I'll do the change, but I'm not sure to get your point. Some tests are
> failing because they are building a path with some components that are valid
> bug empty strings. From your first and second remarks, I get that you want
> to make them still invalid. Is this really what we want ?
> 
> OS.Path.join("a", "b", "c", ""); will throw ?
> 
> Btw Try still fails those tests on windows.

I'm just telling you that you're doing the same test twice so that we can remove the second test.

Comment 21

[:gerard-majax]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #20)
> (In reply to Alexandre LISSY :gerard-majax from comment #18)
> > I'll do the change, but I'm not sure to get your point. Some tests are
> > failing because they are building a path with some components that are valid
> > bug empty strings. From your first and second remarks, I get that you want
> > to make them still invalid. Is this really what we want ?
> > 
> > OS.Path.join("a", "b", "c", ""); will throw ?
> > 
> > Btw Try still fails those tests on windows.
> 
> I'm just telling you that you're doing the same test twice so that we can
> remove the second test.

Sorry, not totally awake yet :)

I've fixed the changes, that Try [https://tbpl.mozilla.org/?tree=Try&rev=9e9ca15eac41] should be okay now, and suiting your remarks. Thanks !

Comment 22

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Updating the fix according to feedback.

Comment 23

[:gerard-majax]

Sorry again, but the suggestion of '!!subpath == false' is of course making xpcshell tests failing, because |!!"" == false| is obviously true.

We really need to decide what we want. Do we want to throw on an empty but valid string?

For reference, Python's behavior:
> >>> print os.path.join("a","b","c")
> a/b/c
> >>> print os.path.join("a","b","c", "")
> a/b/c/
> >>>

Comment 24

[:gerard-majax]

David, I think we should not throw on empty string. What's your decision, based on the previous comment?

Comment 25

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Updated: checking explicitely for null and undefined values.

Comment 26

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Updated: explicitely checking for null || undefined, and make error message not only about undefined.

Comment 27

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

Comment on attachment 8382213 [details] [diff] [review]
0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

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

I have no strong position on empty strings, do whichever you prefer.
Please simplify your tests, though.

::: toolkit/components/osfile/modules/ospath_unix.jsm
@@ +83,4 @@
>    // If there is a path that starts with a "/", eliminate everything before
>    let paths = [];
> +  for each (let subpath in path) {
> +    if (subpath == null || subpath == undefined) {

if (subpath == null)
 is sufficient, as undefined == null

Comment 28

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Updating: fixing a typo (subpath instead of path)

Comment 29

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Updated: only testing against null.

Comment 30

[Brandon Benvie [:benvie]]

Comment on attachment 8382249 [details] [diff] [review]
0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

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

::: toolkit/components/osfile/modules/ospath_unix.jsm
@@ +83,3 @@
>    // If there is a path that starts with a "/", eliminate everything before
>    let paths = [];
> +  for each (let subpath in path) {

for (let subpath of path) {

Comment 31

[:gerard-majax]

(In reply to Brandon Benvie [:benvie] from comment #30)
> Comment on attachment 8382249 [details] [diff] [review]
> 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch
> 
> Review of attachment 8382249 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/modules/ospath_unix.jsm
> @@ +83,3 @@
> >    // If there is a path that starts with a "/", eliminate everything before
> >    let paths = [];
> > +  for each (let subpath in path) {
> 
> for (let subpath of path) {

Why ? What difference does it makes ?

Comment 32

[Brandon Benvie [:benvie]]

(In reply to Alexandre LISSY :gerard-majax from comment #31)
> (In reply to Brandon Benvie [:benvie] from comment #30)
> > Comment on attachment 8382249 [details] [diff] [review]
> > 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch
> > 
> > Review of attachment 8382249 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/osfile/modules/ospath_unix.jsm
> > @@ +83,3 @@
> > >    // If there is a path that starts with a "/", eliminate everything before
> > >    let paths = [];
> > > +  for each (let subpath in path) {
> > 
> > for (let subpath of path) {
> 
> Why ? What difference does it makes ?

`for each` is deprecated Mozilla-specific JS syntax. for-of is ES6 and accomplishes the same thing, so it is preferable. We're trying to not add new uses of Mozilla-specific JS extensions.

Comment 33

[:gerard-majax]

Attachment: 0001-Bug-973538-Throw-an-exception-on-undefined-values-fo.patch

Updating: use "for of" construct.

Comment 34

[:gerard-majax]

Bug 973637 got r+ and should be able to land soon.

Comment 35

[:gerard-majax]

Bug 973637 landed, we can go with this one now.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/170b1052afc5