Bug 191761

Summary: getrlimit on MacOSX
Product: [Developer tools] valgrind Reporter: Dan Kegel <dank>
Component: generalAssignee: Julian Seward <jseward>
Status: RESOLVED FIXED    
Severity: normal CC: mark, njn, rohitrao
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Unlisted Binaries   
OS: macOS   
URL: http://codereview.chromium.org/109052
Latest Commit: Version Fixed In:
Attachments: Patch that seems to fix the problem.
testcase to demonstate problem

Description Dan Kegel 2009-05-06 01:04:05 UTC
Created attachment 33385 [details]
Patch that seems to fix the problem.

Darwin will sometimes set _RLIMIT_POSIX_FLAG on getrlimit/setrlimit calls;
valgrind doesn't currently handle that properly.

Rohit Rao filed this as a bug in the chromium bug tracker, and
came up with a patch for valgrind; I've attached it here.
Comment 1 Nicholas Nethercote 2009-05-07 04:05:43 UTC
You say "Darwin will sometimes set _RLIMIT_POSIX_FLAG"... any more detail about the "sometimes"?

Also, can you attach a test program that fails without the patch?

Thanks.
Comment 2 rohitrao 2009-05-07 04:41:02 UTC
Created attachment 33415 [details]
testcase to demonstate problem

The attached program should show the problem.  Run with something like:
$ gcc rlimit_test.c -o rlimit_test
$ ulimit -a
open files                      (-n) 256
$ ./rlimit_test
RLIMIT_NOFILE is 256
$ valgrind ./rlimit_test
RLIMIT_NOFILE is 266

Valgrind with the patch should output:
RLIMIT_NOFILE is 256
Comment 3 rohitrao 2009-05-07 04:42:55 UTC
(Apologies for the double-mail.  Couldn't figure out how to reply and attach at the same time.)

OS X 10.5 and later ORs in the posix flag.  See http://www.opensource.apple.com/darwinsource/10.5/Libc-498/sys/getrlimit.c .  10.4 and earlier does not do this.  (Thanks to Mark Mentovai for finding that.)
Comment 4 Nicholas Nethercote 2009-05-07 05:19:52 UTC
In coregrind/m_libcproc.c we have this:

Int VG_(setrlimit) (Int resource, const struct vki_rlimit *rlim)
{
   SysRes res;
   /* res = setrlimit( resource, rlim ); */
   res = VG_(do_syscall2)(__NR_setrlimit, resource, (UWord)rlim);
   return sr_isError(res) ? -1 : sr_Res(res);
}

Should that be adjusted too?
Comment 5 Nicholas Nethercote 2009-05-07 05:56:38 UTC
To answer my own question: no.  The kernel masks out the posix flag if present.
(Its presence/non-presence changes some of the behaviour of the syscall, but not in any way that affects us.)
Comment 6 Nicholas Nethercote 2009-05-07 06:19:35 UTC
Patch and test committed as r9790 on the DARWIN branch.  Thanks.
Comment 7 Mark Mentovai 2009-05-07 06:21:49 UTC
Depends on who calls VG_(setrlimit).

The getrlimit and setrlimit syscall wrappers in libSystem, when in POSIX-conformant mode, OR in the strict POSIX bit.  Anything on the "far" (kernel) side of that needs to be prepared for that bit to be set.  Anything that replaces the libSystem syscall wrappers (bypassing them) should implement similar logic.

I don't know what VG_(setrlimit) is.  If it's replacing the libSystem setrlimit syscall wrapper, then it should mimic what's done in libSystem: it should set the strict POSIX bit before doing the syscall, but only in POSIX-conformant mode.  Unfortunately, that last bit might complicate matters: the POSIX compliance level is selected at compile time, and winds up baked into the object code by symbol: POSIX conformance uses _setrlimit$UNIX2003, and compatibility mode uses undecorated _setrlimit.  'Course, it's possible that VG_(setrlimit) does something entirely different.

If VG_(setrlimit) is in fact a replacement for libSystem's setrlimit syscall wrapper, then ignoring the issue in this function will possibly result in subtly different behavior for this syscall in Valgrind and uninstrumented.
Comment 8 Nicholas Nethercote 2009-05-07 06:30:52 UTC
VG_(setrlimit)() is used within Valgrind itself.  It's not used at all to emulate setrlimit() in the client.  So I think we're safe.
Comment 9 Mark Mentovai 2009-05-07 06:31:35 UTC
Great, then I agree.  Thanks.
Comment 10 Nicholas Nethercote 2009-06-25 07:12:01 UTC
Looks like I forgot to close this one.  Doing so now.