110769 - nsLocalFileUnix: somewhat improper use of realpath

Status: NEW

(Core :: XPCOM, defect, P3)

Description

[tenthumbs]

nsLocalFile::Normalize uses the realpath system call but doesn't
properly consider the possible errors. In particular, realpath will
return -1 and errno will be ENOENT if the input path does not exist, but
it will also return ENOENT for a dangling symlink. Since all users are
explicitly allowed to create dangling symlinks, this needs to be fixed.

If it were me, I would test for ENOENT and, if the input is a symlink,
just copy the input to the resolved path. Yes, realpath will return a
partially resolved path but that's not very useful since you can't know
which parts were resolved without separating the input into its
components and trying each one in succession. Generally not worth the
trouble.

I suspect, however, that mozilla has some wrong-headed idea about
symlinks so this simple fix won't work.

Comment 1

[Doug Turner (:dougt)]

comments from unix hackers?

Comment 2

[Mike Shaver (:shaver emeritus)]

That plan sounds reasonable.  (I recall not wanting ::Normalize to resolve
symlinks at all, but I don't remember the details.)

Comment 3

[Christopher Blizzard (:blizzard)]

Yeah, I don't remember much about this either. ( Did I write that code? )

Comment 4

[Brendan Eich [:brendan]]

shaver: if you want Normalize to eliminate /../ components, then it must
consider directory symlinks.

tenthumbs: agreed on leaving dangling symlinks denormal and as input.

/be

Comment 5

[tenthumbs]

I think you just want to copy the input to the output on any ENOENT
case, a name that doesn't exist is still a name. There are problems,
though. An un-normalized name can be longer than PATH_MAX so buffer
overruns are a possibility. Other code seems to use MAXPATHLEN so this
may be a real problem, or maybe not.

Are there any specs on what mozilla expects from these methods? They all
appear to be rather ad hoc solutions.

BTW, shouldn't the internal buffer be
  char resolved_path[PATH_MAX + 1] = "";
and after realpath
  resolved_path[PATH_MAX] = '\0'; ?

Comment 7

[Alex Vincent [:WeirdAl]]

Please note bug 349094 comment 3.  I posted a wild-guess patch there for Mac, as I did not have a Linux box at the time.  I would like to see some progress on this.  :-)

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #5)
> Are there any specs on what mozilla expects from these methods?

There are specs in nsI(Local)?File.idl, of course, but they're often so vague as to be useless.  We can improve where possible without changing behaviors clients have come to expect, but I hold no hope for having sane, completely specified and implemented file classes prior to Mozilla 2.  I tend to think this case is just one example, and I'd be hesitant to fix it before Mozilla 2.

Comment 9

[Jeff Walden [:Waldo]]

Or, I suppose more correctly, it's not worth fixing it before then, because it provides a guarantee that isn't all that useful with respect to the rest of nsILocalFile, when one takes into account how inconsistent and unspecified nsILocalFile is.

Comment 10

[Benjamin Smedberg]

Yeah, localfile is on my mozilla2-whack list: in particular, if we're going to keep these heavyweight file objects (instead of string paths), we should make them immutable.