Open Bug 550463 Opened 12 years ago Updated 11 years ago
_Load CFBundle should pass null to realpath
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?
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: 12 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
You need to log in before you can comment on or make changes to this bug.