550463 - pr_LoadCFBundle should pass null to realpath

Status: REOPENED

(NSPR :: NSPR, defect)

Description

[timeless]

617     char pathBuf[PATH_MAX];
618     const char *resolvedPath;
619     CFStringRef pathRef;
620 
621     /* Takes care of relative paths and symlinks */
622     resolvedPath = realpath(name, pathBuf);

The OS X man page for realpath says:
REALPATH(3)              BSD Library Functions Manual              REALPATH(3)

     char *
     realpath(const char *restrict file_name, char *restrict resolved_name);

     As a permitted extension to the standard, if resolved_name is NULL, memory
     is allocated for the resulting absolute pathname, and is returned by
     realpath().  This memory should be freed by a call to free(3) when no longer
     needed.

This allows realpath to handle paths that are longer than PATH_MAX and makes Coverity and friends much happier.

The Linux man page for realpath has this in its Bugs section:

     Avoid using this function.  It is broken by design

Since the OS X version can be used in a non broken manner, it would be good if it was.

Actually, it'd be nice if we had a function which relied on a configure test to determine whether 'realpath(x, NULL)' works, and if so always used it, and if not, implemented realpath 'safely'.

Comment 1

[timeless]

Attachment: use the better realpath (backed out)

Comment 2

[Wan-Teh Chang]

Comment on attachment 430603 [details] [diff] [review]
use the better realpath (backed out)

r=wtc.  The original code is correct because the output buffer is large enough:
http://www.opengroup.org/onlinepubs/007908799/xsh/realpath.html
If Coverity warns about this, I can submit a bug report so they can fix
their "model" for realpath.

Comment 3

[timeless]

Coverity generally doesn't seem to model this stuff well, no. i haven't checked to see if it flagged this instance. that said, i'd hope we can replace all uses of the original style with something that works better. this one seems to be the easiest because Apple has provided a non lame flavor.

please commit this, as i don't have CVS permission for NSPR.

Comment 4

[Wan-Teh Chang]

I checked in the patch on the NSPR trunk (NSPR 4.8.5).

Checking in prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.106; previous revision: 3.105
done

Comment 5

[timeless]

:(, it turns out this is a 10.6 feature, and some of our boxes are 10.5.

Comment 6

[timeless]

http://hg.mozilla.org/mozilla-central/rev/7b2b19c142c2

ted: please back this commit out

Comment 7

[Wan-Teh Chang]

Comment on attachment 430603 [details] [diff] [review]
use the better realpath (backed out)

I backed out this patch on the NSPR trunk (NSPR 4.8.5).

Checking in prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.107; previous revision: 3.106
done

Comment 8

[Zack Weinberg (:zwol)]

It really would be better to find a way to use the second-argument-NULL form where possible.  I'll quote the full discussion from the Linux manpage:

       The POSIX.1-2001 standard version of this function is broken by design,
       since it is impossible to determine a suitable size for the output buf‐
       fer,  resolved_path.   According  to  POSIX.1-2001  a  buffer  of  size
       PATH_MAX suffices, but PATH_MAX need not be a defined constant, and may
       have to be obtained using pathconf(3).  And asking pathconf(3) does not
       really help, since, on the one hand POSIX  warns  that  the  result  of
       pathconf(3) may be huge and unsuitable for mallocing memory, and on the
       other hand pathconf(3) may return -1 to signify that  PATH_MAX  is  not
       bounded.    The  resolved_path == NULL  feature,  not  standardized  in
       POSIX.1-2001, but standardized  in  POSIX.1-2008,  allows  this  design
       problem to be avoided.

Comment 9

[(not currently active) Ted Mielczarek]

Presumably we could write a configure test for it, but I'm guessing it'd require actually running code, which sucks for cross-compiling.

Comment 10

[Wan-Teh Chang]

Perhaps we don't need to call realpath.  Apple's documentation of
CFURLCreateWithFileSystemPath implies the filePath argument doesn't
need to be an absolute pathname:

http://developer.apple.com/mac/library/documentation/corefoundation/Reference/CFURLRef/Reference/reference.html#//apple_ref/c/func/CFURLCreateWithFileSystemPath

Comment 11

[timeless]

Attachment: remove the realpath stuff

this is what i currently have; posting it as a checkpoint

Comment 12

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.