Closed Bug 973538 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: OS.File, defect)

30 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 9 obsolete files)

2.21 KB, patch
gerard-majax
: review+
Details | Diff | Splinter Review
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.
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.
Component: General → OS.File
Product: Core → Toolkit
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.
Depends on: 973637
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.
Status: NEW → ASSIGNED
(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 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 :)
Attachment #8377142 - Flags: review?(dteller)
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
Attachment #8377142 - Flags: review?(dteller) → feedback+
(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.
Flags: needinfo?(dteller)
(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.
Flags: needinfo?(dteller)
Updating: should address all previous remarks (TypeError, using rest arguments)
Attachment #8377142 - Attachment is obsolete: true
Attachment #8381423 - Flags: review?(dteller)
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|.
Attachment #8381423 - Flags: review?(dteller) → review+
(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.
Addressing latest nits, keeping r+.
Attachment #8381423 - Attachment is obsolete: true
Attachment #8381625 - Flags: review+
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?
Flags: needinfo?(dteller)
Might as well do it now.
Flags: needinfo?(dteller)
We get into a join("/some/correct/path", ""). I suspect we want to handle this gracefully and not throw in this case.
Updated patch. Maybe we do not want to do the paths.push(subpath) if subpath.length == 0 ?
Attachment #8381625 - Attachment is obsolete: true
Attachment #8381772 - Flags: review?(dteller)
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.
Attachment #8381772 - Flags: review?(dteller) → review+
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.
Flags: needinfo?(dteller)
Try is still failing on windows because of the same kind of behavior for winGetDrive("directory", "").
(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.
Flags: needinfo?(dteller)
(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 !
Updating the fix according to feedback.
Attachment #8381772 - Attachment is obsolete: true
Attachment #8382115 - Flags: review+
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/
> >>>
David, I think we should not throw on empty string. What's your decision, based on the previous comment?
Flags: needinfo?(dteller)
Updated: checking explicitely for null and undefined values.
Attachment #8382115 - Attachment is obsolete: true
Attachment #8382200 - Flags: review+
Updated: explicitely checking for null || undefined, and make error message not only about undefined.
Attachment #8382200 - Attachment is obsolete: true
Attachment #8382213 - Flags: review+
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
Updating: fixing a typo (subpath instead of path)
Attachment #8382213 - Attachment is obsolete: true
Attachment #8382247 - Flags: review+
Updated: only testing against null.
Attachment #8382247 - Attachment is obsolete: true
Attachment #8382249 - Flags: review+
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) {
(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 ?
(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.
Updating: use "for of" construct.
Attachment #8382249 - Attachment is obsolete: true
Attachment #8382337 - Flags: review+
Flags: needinfo?(dteller)
Bug 973637 got r+ and should be able to land soon.
Bug 973637 landed, we can go with this one now.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/170b1052afc5
Keywords: checkin-needed
Whiteboard: [systemsfe] → [systemsfe][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/170b1052afc5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [systemsfe][fixed-in-fx-team] → [systemsfe]
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.