Open Bug 550463 Opened 10 years ago Updated 10 years ago

pr_LoadCFBundle should pass null to realpath

Categories

(NSPR :: NSPR, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

REOPENED

People

(Reporter: timeless, Assigned: timeless)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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'.
Assignee: wtc → timeless
Status: NEW → ASSIGNED
Attachment #430603 - Flags: review?
Attachment #430603 - Flags: review? → review?(wtc)
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.
Attachment #430603 - Flags: review?(wtc) → review+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8.5
:(, it turns out this is a 10.6 feature, and some of our boxes are 10.5.
http://hg.mozilla.org/mozilla-central/rev/7b2b19c142c2

ted: please back this commit out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #430603 - Attachment description: use the better realpath → use the better realpath (backed out)
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.
Presumably we could write a configure test for it, but I'm guessing it'd require actually running code, which sucks for cross-compiling.
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
this is what i currently have; posting it as a checkpoint
Attachment #430603 - Attachment is obsolete: true
Target Milestone: 4.8.5 → 4.8.6
Target Milestone: 4.8.6 → 4.8.7
Target Milestone: 4.8.7 → ---
You need to log in before you can comment on or make changes to this bug.