Closed Bug 764436 Opened 8 years ago Closed 8 years ago

[OS.File] Directory traversal

Categories

(Toolkit :: OS.File, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(8 files, 31 obsolete files)

6.04 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
7.03 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
5.33 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
5.00 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.28 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
8.45 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
8.05 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
992 bytes, patch
Yoric
: review+
Details | Diff | Splinter Review
Attached file Possible API for file traversal (obsolete) —
We need a directory traversal mechanism for OS.File. As usual, we will start with a worker thread-only synchronous version.

Attaching a possible API.
Attachment #632721 - Flags: feedback?(ttaubert)
OS: Mac OS X → All
Hardware: x86 → All
Attachment #632721 - Attachment is patch: true
Attachment #632721 - Attachment mime type: application/x-javascript → text/plain
Comment on attachment 632721 [details]
Possible API for file traversal

Oops, my bad.
Attachment #632721 - Attachment is patch: false
Attachment #632721 - Attachment mime type: text/plain → application/x-javascript
Comment on attachment 632721 [details]
Possible API for file traversal

This looks pretty good to me.
Attachment #632721 - Flags: feedback?(ttaubert) → feedback+
Attached patch Directory-related constants (obsolete) — Splinter Review
Assignee: nobody → dteller
Attached patch Unix front-end (obsolete) — Splinter Review
Attached patch Windows back-end (obsolete) — Splinter Review
Attached patch Unix back-end (obsolete) — Splinter Review
Comment on attachment 633152 [details] [diff] [review]
Unix front-end

Taras, could you tell me what you think of this API?
Attachment #633152 - Flags: review?(taras.mozilla)
Comment on attachment 633152 [details] [diff] [review]
Unix front-end

i think 
if (!this._dir) return; checks are redundant. Things will just fail if you attempt to do stuff after close() anyway. But I don't have strong feelings about this

+       /**
+        * |true| if the entry is a special directory, |false| otherwise
+        */
+       get isSpecialDir() {
+         return this.name == "." || this.name == "..";
+       }, 


I think we should just skip . and .. in directory listings and get rid of this.

+       get name() {
+         delete this.name;
+         let name = this._entry.d_name.readString();
+         Object.defineProperty(this, "name", {value: name});
+         return name;
+       }

What's going on here, is this meant to be a lazy getter?

r- because I want to see '.' & '..' removed.
Attachment #633152 - Flags: review?(taras.mozilla) → review-
(In reply to Taras Glek (:taras) from comment #10)
> Comment on attachment 633152 [details] [diff] [review]
> Unix front-end
> 
> i think 
> if (!this._dir) return; checks are redundant. Things will just fail if you
> attempt to do stuff after close() anyway. But I don't have strong feelings
> about this
> 
> +       /**
> +        * |true| if the entry is a special directory, |false| otherwise
> +        */
> +       get isSpecialDir() {
> +         return this.name == "." || this.name == "..";
> +       }, 
> 
> 
> I think we should just skip . and .. in directory listings and get rid of
> this.

Ok.

> +       get name() {
> +         delete this.name;
> +         let name = this._entry.d_name.readString();
> +         Object.defineProperty(this, "name", {value: name});
> +         return name;
> +       }
> 
> What's going on here, is this meant to be a lazy getter?

This is a lazy getter compatible with "use strict".
Attached patch Windows front-end (obsolete) — Splinter Review
Attachment #633151 - Attachment is obsolete: true
Attached patch Unix front-end (obsolete) — Splinter Review
Attachment #633152 - Attachment is obsolete: true
Attachment #634823 - Flags: review?(taras.mozilla)
Attached patch Unix back-end (obsolete) — Splinter Review
Here we go. The new version filters out "." and "..", and can return the full name of the file examined.
Attachment #633154 - Attachment is obsolete: true
Attachment #634824 - Flags: review?(taras.mozilla)
Attachment #633155 - Attachment is patch: true
Attached patch Windows front-end (obsolete) — Splinter Review
Attachment #634822 - Attachment is obsolete: true
Attached patch Windows back-end (obsolete) — Splinter Review
Attachment #633153 - Attachment is obsolete: true
Comment on attachment 634824 [details] [diff] [review]
Unix back-end

How does .OSFILE_OFFSET_D_TYPE get defined?
Comment on attachment 634824 [details] [diff] [review]
Unix back-end

This magic needs heavy commenting
+
+       {
+         let dirent_struct = [];
+         if (OS.Constants.libc.OSFILE_OFFSET_D_TYPE != 0) {
+           dirent_struct.push({padding_before_d_type: ctypes.ArrayType(ctypes.uint8_t, OS.Constants.libc.OSFILE_OFFSET_D_TYPE)});
+         }
+         dirent_struct.push({d_type: ctypes.uint8_t});
+         if (OS.Constants.libc.OSFILE_OFFSET_D_NAME > OS.Constants.libc.OSFILE_OFFSET_D_TYPE + 1) {
+           dirent_struct.push({padding_before_d_name: ctypes.ArrayType(ctypes.uint8_t, OS.Constants.libc.OSFILE_OFFSET_D_NAME - OS.Constants.libc.OSFILE_OFFSET_D_TYPE - 1)});
+         }
+         dirent_struct.push({d_name:ctypes.ArrayType(ctypes.char, OS.Constants.libc.OSFILE_SIZEOF_D_NAME)});
+       Types.dirent =
+         new Type("dirent",
+           ctypes.StructType("dirent", dirent_struct));
+         LOG("dirent is: " + Types.dirent.implementation.toSource());
+       }


+       UnixFile.readdir =
+         declareFFI("readdir$INODE64", ctypes.default_abi,
+                     /*return*/Types.null_or_dirent_ptr,
+                     /*dir*/   Types.DIR.in_ptr) // For MacOS X
+        || declareFFI("readdir", ctypes.default_abi,
+                    /*return*/Types.null_or_dirent_ptr,
+                    /*dir*/   Types.DIR.in_ptr); // Other Unices

really? Can we either flip this logic so it works on normal unix without ifdefing or use the C preprocessor on this file(i prefer this option). This might work better for above dirent magic too.
Comment on attachment 634824 [details] [diff] [review]
Unix back-end

forgot to r- in previous comment
Attachment #634824 - Flags: review?(taras.mozilla) → review-
Attachment #634823 - Flags: review?(taras.mozilla) → review+
(In reply to Taras Glek (:taras) from comment #17)
> Comment on attachment 634824 [details] [diff] [review]
> Unix back-end
> 
> How does .OSFILE_OFFSET_D_TYPE get defined?

It comes from patch "Directory-related constants". It is an |offsetof|.

> This magic needs heavy commenting

Definitely.

> really? Can we either flip this logic so it works on normal unix without ifdefing or use the C preprocessor on this file(i prefer this option). This might work better for above dirent magic too.

Yes, really. Finding out the symbol name for MacOS X was lots of fun - they actually use assembly-level tricks to get this compiled.

I personally dislike (a lot) using the C preprocessor on JavaScript, because:
- it completely kills line numbers, hence making debugging much more complicated;
- in our build system, it is actually not the C preprocessor but a much less powerful ersatz that really cannot do much;
- this splits the logic across yet another file (either configure.in or Makefile.in).

Let me produce an alternative, you will tell me if you like it better.
Attached patch Unix back-end (obsolete) — Splinter Review
Clarifying and documenting the definition of |dirent|.
Note that we are going to have the same issues with |stat| in bug 766194.
Attachment #634824 - Attachment is obsolete: true
Attachment #635274 - Flags: review?(taras.mozilla)
Attached patch Directory-related constants (obsolete) — Splinter Review
So, Taras, this is where the constants come from. I have added a little more documentation and macro _DARWIN_FEATURE_64_BIT_INODE to clarify a little the definition of |readdir|/|readdir$INODE64|.

Note that we will use the same macro for |stat|.
Attachment #633150 - Attachment is obsolete: true
Attachment #635277 - Flags: review?(taras.mozilla)
Attached patch Windows back-end (obsolete) — Splinter Review
Attachment #634827 - Attachment is obsolete: true
Attached patch Windows front-end (obsolete) — Splinter Review
Attachment #634826 - Attachment is obsolete: true
Attachment #635274 - Flags: review?(taras.mozilla) → review+
Attached patch Unix front-end (obsolete) — Splinter Review
Attachment #634823 - Attachment is obsolete: true
Attachment #635719 - Flags: review+
Attached patch Windows front-end (obsolete) — Splinter Review
Attachment #635297 - Attachment is obsolete: true
Attachment #635720 - Flags: review?(taras.mozilla)
Attached patch Windows back-end (obsolete) — Splinter Review
Attachment #635296 - Attachment is obsolete: true
Attachment #635721 - Flags: review?(taras.mozilla)
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #633155 - Attachment is obsolete: true
Attachment #635722 - Flags: review?(taras.mozilla)
Attached patch Unix front-end (obsolete) — Splinter Review
Small fix.
Attachment #635719 - Attachment is obsolete: true
David, I likely wont be able to get to these reviews this week. Either ask someone else for review or hold tight :)
Attached patch Shared code (obsolete) — Splinter Review
After some "interesting" debugging, I have done a little refactoring on the definition of |dirent|, so I am attaching two patches for this purpose.
Attachment #635274 - Attachment is obsolete: true
Attachment #636677 - Flags: review?(taras.mozilla)
Attached patch Directory-related constants (obsolete) — Splinter Review
Attachment #635277 - Attachment is obsolete: true
Attachment #635277 - Flags: review?(taras.mozilla)
Attachment #636678 - Flags: review?(taras.mozilla)
Attached patch Unix back-end (obsolete) — Splinter Review
Attachment #636679 - Flags: review?(taras.mozilla)
Attachment #636679 - Flags: review?(doug.turner)
Attachment #636677 - Flags: review?(doug.turner)
Attachment #636678 - Flags: review?(doug.turner)
Attached patch Windows front-end (obsolete) — Splinter Review
Attachment #635720 - Attachment is obsolete: true
Attachment #635720 - Flags: review?(taras.mozilla)
Attachment #636683 - Flags: review?(taras.mozilla)
Attachment #636683 - Flags: review?(doug.turner)
Attached file Companion testsuite (obsolete) —
Attachment #635722 - Attachment is obsolete: true
Attachment #635722 - Flags: review?(taras.mozilla)
Attachment #636684 - Flags: review?(taras.mozilla)
Attachment #636684 - Flags: review?(doug.turner)
Comment on attachment 636677 [details] [diff] [review]
Shared code

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

::: toolkit/components/osfile/osfile_shared.jsm
@@ +417,5 @@
> +       if (!size || size < 0) {
> +         throw new TypeError("HollowStructure expects a (positive) size");
> +       }
> +       this.offset_to_field_info = [];
> +       this.name = name;

Why are some members prefixed with _

@@ +419,5 @@
> +       }
> +       this.offset_to_field_info = [];
> +       this.name = name;
> +       this._size = size;
> +       this._paddings = 0;

why do we need paddings?  Is it singular or plural?

@@ +430,5 @@
> +        * @param {string} name The name of the field.
> +        * @param {CType|Type} type The type of the field.
> +        */
> +       add_field_at: function add_field_at(offset, name, type) {
> +         if (type instanceof Type) {

maybe move below the tests so that you don't assign this on the parameter checking failures.
(In reply to Doug Turner (:dougt) from comment #36)
> Comment on attachment 636677 [details] [diff] [review]
> Shared code
> 
> Review of attachment 636677 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_shared.jsm
> @@ +417,5 @@
> > +       if (!size || size < 0) {
> > +         throw new TypeError("HollowStructure expects a (positive) size");
> > +       }
> > +       this.offset_to_field_info = [];
> > +       this.name = name;
> 
> Why are some members prefixed with _

You are right, this is probably not needed.

> @@ +419,5 @@
> > +       }
> > +       this.offset_to_field_info = [];
> > +       this.name = name;
> > +       this._size = size;
> > +       this._paddings = 0;
> 
> why do we need paddings?  Is it singular or plural?

This is simply a counter, to call the first padding field "padding_0", the second "padding_1", etc.

> 
> @@ +430,5 @@
> > +        * @param {string} name The name of the field.
> > +        * @param {CType|Type} type The type of the field.
> > +        */
> > +       add_field_at: function add_field_at(offset, name, type) {
> > +         if (type instanceof Type) {
> 
> maybe move below the tests so that you don't assign this on the parameter
> checking failures.

Good point.
Attached patch Shared code (obsolete) — Splinter Review
Applied feedback.
Attachment #636677 - Attachment is obsolete: true
Attachment #636677 - Flags: review?(taras.mozilla)
Attachment #636677 - Flags: review?(doug.turner)
Attachment #637060 - Flags: review?(doug.turner)
Attachment #635721 - Flags: review?(taras.mozilla) → review+
Attachment #636678 - Flags: review?(taras.mozilla)
Attachment #636678 - Flags: review?(doug.turner)
Attachment #636678 - Flags: review+
Attachment #636679 - Flags: review?(taras.mozilla)
Attachment #636679 - Flags: review?(doug.turner)
Attachment #636679 - Flags: review+
Comment on attachment 636683 [details] [diff] [review]
Windows front-end

I would prefer s/fileTimeToDate/FILETIME_to_Date/, this way it's more clear which filetime

 throw new File.Error("iter", error) <-- mention FindFirstFile in the error message


Good work
Attachment #636683 - Flags: review?(taras.mozilla)
Attachment #636683 - Flags: review?(doug.turner)
Attachment #636683 - Flags: review+
Attachment #636684 - Flags: review?(taras.mozilla)
Attachment #636684 - Flags: review?(doug.turner)
Attachment #636684 - Flags: review+
Attachment #637060 - Flags: review?(doug.turner) → review+
Attached patch Directory-related constants (obsolete) — Splinter Review
Attachment #632721 - Attachment is obsolete: true
Attachment #636678 - Attachment is obsolete: true
Attachment #638723 - Flags: review+
Attached patch Shared codeSplinter Review
Attachment #637060 - Attachment is obsolete: true
Attachment #638726 - Flags: review+
Attached patch Unix back-endSplinter Review
Attachment #636679 - Attachment is obsolete: true
Attachment #638727 - Flags: review+
Attached patch Unix front-endSplinter Review
Attachment #635766 - Attachment is obsolete: true
Attachment #638728 - Flags: review+
Attached patch Windows back-end (obsolete) — Splinter Review
Attachment #635721 - Attachment is obsolete: true
Attachment #638729 - Flags: review+
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #636684 - Attachment is obsolete: true
Attachment #638731 - Flags: review+
Attached patch Windows front-end (obsolete) — Splinter Review
Attachment #636683 - Attachment is obsolete: true
Attachment #638732 - Flags: review+
Attachment #638723 - Attachment is obsolete: true
Attachment #638974 - Flags: review+
Attachment #638731 - Attachment is obsolete: true
Attachment #639181 - Flags: review-
Attachment #638732 - Attachment is obsolete: true
Attachment #639182 - Flags: review+
Attached patch Windows back-endSplinter Review
Attachment #638729 - Attachment is obsolete: true
Attachment #639183 - Flags: review+
Attachment #639181 - Flags: review- → review+
Will check this in \o/.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla16
Just a heads up, i'll investigate more later, but it seems that the OSFileConstants.cpp commit (https://hg.mozilla.org/mozilla-central/rev/844c43a98031)
broke my builds : http://buildbot.rhaalovely.net/builders/mozilla-central-i386/builds/220/steps/build/logs/stdio

OSFileConstants.cpp:278: error: 'DT_WHT' was not declared in this scope

If i look at our sys/dirent.h (http://fxr.watson.org/fxr/source/sys/dirent.h?v=OPENBSD#L65) it shows that DT_WHT doesnt exist.

If i look at the FreeBSD/NetBSD headers, DT_WHT is here, so i'll see what the standard says wrt those #defines and if it's missing on our side, or if it'd have to be #ifdefed in OSFileConstants.cpp...

Looking at history in our mailing lists, it seems it wont come to sys/dirent.h .. http://old.nabble.com/Re%3A-add-missing-DT_WHT-in-sys-dirent.h-p31216633.html

Finally, note that if i look at a linux manpage for readdir (http://linux.die.net/man/3/readdir) it doesn't mention DT_WHT.
And after a bit of discussion with our standards peers :

- whiteouts were to support union mounts,  ie the ability to remove a file from a union without removing it from the underlying unioned fs - OpenBSD of course removed support for union mounts ages ago.

- DT_WHT is not standard at all, posix doesn't even standardize *any* of the DT_*

- DT_WHT was only used in return values of readdir2, which was a BSD4ism that existed *just* to support DT_WHT and union filesystems

OS.File only calls plain readdir(), so there's no chance for it to get a value with a DT_WHT type.

So here we have the following choices :
- remove the constant definition for DT_WHT (since it's non-standard and legacy ?)
- wrap it within #ifndef (__OpenBSD__) (ugly!)
- wrap it within #if defined(DT_WHT) (probably the better approach..)
I think that we should completely remove DT_WHT for the moment, until we have a usage scenario. Can you handle this or would you prefer if I do?
Sure, here's a patch that fixes the build for me
Attachment #639961 - Flags: review?(dteller)
Comment on attachment 639961 [details] [diff] [review]
remove unused dt_wth

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

Looks good to me.
Thanks for the patch!
Attachment #639961 - Flags: review?(dteller) → review+
No longer blocks: 761138
Component: Networking: File → OS.File
Product: Core → Toolkit
You need to log in before you can comment on or make changes to this bug.