Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17066002:
attempt to make the libc urandom override work for non-glibc too (Closed)
|
Created:
7 years, 6 months ago by Mostyn Bramley-Moore Modified:
7 years, 5 months ago CC:
chromium-reviews, agl, jln+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionattempt to make the libc urandom override work for non-glibc too
This patch attempts to generalise the linux libc urandom override, so
that it works for non-glibc linux platforms too.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208924
Patch Set 1 #
Total comments: 4
Patch Set 2 : not finding *stat64 should be fatal #Patch Set 3 : change TODO note into a regular comment #Patch Set 4 : split stat overrides into two distinct cases #
Total comments: 4
Patch Set 5 : fix style nits #Patch Set 6 : remove ifdefs #
Total comments: 14
Patch Set 7 : use fstat* instead of __fxstat* for simplicity #
Total comments: 2
Patch Set 8 : add back some ifdefs, but don't completely disable stat override if xstat is available #Patch Set 9 : undo changes no longer required #Patch Set 10 : make InitLibcUrandomOverrides call InitLibcFileIOFunctions #Patch Set 11 : fixup of previous commit, call the function once only #Patch Set 12 : comment improvement #Patch Set 13 : remove obsolete comment #
Total comments: 13
Patch Set 14 : go back to two distinct ifdef cases #Patch Set 15 : undo copyright year change #Messages
Total messages: 36 (0 generated)
@jln, agl: after looking at this some more, I think I have found a reasonable way to generalise this- how does this look to you?
Oh, and note the actual use case for this code...
In mozilla/security/nss/lib/freebl/unix_rand.c:
RNG_SystemInfoForRNG() calls RNG_FileUpdate("/dev/urandom", ...) which calls
stat().
This looks good in general. Adam: please let me know if there is a reason I missed for handling missing *stat64 function with a simple warning. https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... File sandbox/linux/services/libc_urandom_override.cc (right): https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... sandbox/linux/services/libc_urandom_override.cc:88: LOG(WARNING) << "Failed to get stat64() from libc."; It looks like the rationale for doing a LOG(WARNING) rather than LOG(FATAL) above is that there is a fallback from fopen64 to fopen. Since it's not the case here, please LOG(FATAL). (In general I would rather discourage the use of untested fallbacks). For consistency, please patch the LOG(WARNING) below as well. Unless of course we know that for some reason I missed we recover from missing *64 functions gracefully.
On 2013/06/14 09:59:28, Mostyn Bramley-Moore wrote:
> Oh, and note the actual use case for this code...
>
> In mozilla/security/nss/lib/freebl/unix_rand.c:
> RNG_SystemInfoForRNG() calls RNG_FileUpdate("/dev/urandom", ...) which calls
> stat().
Ryan: any chance we could change NSS to fstat instead of stat in parallel to
this change?
lgtm, but I'm not an owner https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... File sandbox/linux/services/libc_urandom_override.cc (right): https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... sandbox/linux/services/libc_urandom_override.cc:74: // TODO(sergeyu, mostynb): Hopefully the stat/xstat overrides can work nit: This TODO seems to be too vague to be actionable. Maybe remove TODO() and leave the rest of the comment as is.
wtc was just working on this code on the NSS side, he'd be a better person to comment. Regardless of the feasibility of an NSS change, we'll have to support the stat() approach until we require an NSS version >= the version with that change (on Linux). Which is to say, it'll be a while :)
https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... File sandbox/linux/services/libc_urandom_override.cc (right): https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... sandbox/linux/services/libc_urandom_override.cc:74: // TODO(sergeyu, mostynb): Hopefully the stat/xstat overrides can work On 2013/06/14 20:13:47, Sergey Ulanov wrote: > nit: This TODO seems to be too vague to be actionable. Maybe remove TODO() and > leave the rest of the comment as is. Done. https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... sandbox/linux/services/libc_urandom_override.cc:88: LOG(WARNING) << "Failed to get stat64() from libc."; On 2013/06/14 19:43:16, Julien Tinnes wrote: > It looks like the rationale for doing a LOG(WARNING) rather than LOG(FATAL) > above is that there is a fallback from fopen64 to fopen. > > Since it's not the case here, please LOG(FATAL). (In general I would rather > discourage the use of untested fallbacks). > > For consistency, please patch the LOG(WARNING) below as well. > > Unless of course we know that for some reason I missed we recover from missing > *64 functions gracefully. I made this change in patchset 2, but now I think we might need to wrap the *64 functions in __USE_LARGEFILE64 ifdefs- though I need to see if this will work for freebsd/openbsd too first.
On 2013/06/14 20:38:22, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... > File sandbox/linux/services/libc_urandom_override.cc (right): > > https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... > sandbox/linux/services/libc_urandom_override.cc:74: // TODO(sergeyu, mostynb): > Hopefully the stat/xstat overrides can work > On 2013/06/14 20:13:47, Sergey Ulanov wrote: > > nit: This TODO seems to be too vague to be actionable. Maybe remove TODO() and > > leave the rest of the comment as is. > > Done. > > https://codereview.chromium.org/17066002/diff/1/sandbox/linux/services/libc_u... > sandbox/linux/services/libc_urandom_override.cc:88: LOG(WARNING) << "Failed to > get stat64() from libc."; > On 2013/06/14 19:43:16, Julien Tinnes wrote: > > It looks like the rationale for doing a LOG(WARNING) rather than LOG(FATAL) > > above is that there is a fallback from fopen64 to fopen. > > > > Since it's not the case here, please LOG(FATAL). (In general I would rather > > discourage the use of untested fallbacks). > > > > For consistency, please patch the LOG(WARNING) below as well. > > > > Unless of course we know that for some reason I missed we recover from missing > > *64 functions gracefully. > > I made this change in patchset 2, but now I think we might need to wrap the *64 > functions in __USE_LARGEFILE64 ifdefs- though I need to see if this will work > for freebsd/openbsd too first. I kicked a 32 bits bot, let's see what it says. For FreeBSD / OpenBSD, I wouldn't worry: I don't think they would even need that code as the ports don't have a sandbox.
> > I made this change in patchset 2, but now I think we might need to wrap the > *64 > > functions in __USE_LARGEFILE64 ifdefs- though I need to see if this will work > > for freebsd/openbsd too first. > > I kicked a 32 bits bot, let's see what it says. 32bit installs can support *64 too (the compiler doesn't have to use a native datatype for the 64bit values). I checked a few toolchains that I had handy: * On 64 bit ubuntu 12.10 (gcc 4.7.2 eglibc 2.15): gcc does not include *64 in the headers unless you define __USE_LARGEFILE64 before including sys/stat.h, but g++ does define __USE_LARGEFILE64 and include *64. * 32 bit Debian 6.0.0 (gcc 4.4.5 glibc 2.11) behaves the same. * A mipsel toolchain for an embedded device (gcc 4.5.3 uClibc 0.9.32) neither gcc and g++ define __USE_LARGEFILE64 but *64 always seems to be available. In summary, since this is a c++ file I think it's safe to assume that the *64 functions will always be declared in the header files and defined in the C library, and we don't need __USE_LARGEFILE64 ifdefs. However, while the linux_rel_precise32 job built OK, it failed to dlsym stat at runtime: [0614/142357:FATAL:libc_urandom_override.cc(85)] Failed to get stat() from libc. I will have to do some more experimenting :/
I checked a 32bit ubuntu precise (12.04) install, libc does not provide stat* symbols- instead stat is inlined as __stat, which calls __xstat. In patchset 4 I split the cases in two distinct cases (with/without xstat), to minimise the number of (slow?) dlsym calls. I still need to test this on a uClibc target, so it's not ready for merging yet, but is worth running some linux try jobs in the meantime.
@Julien: I successfully tested patchset 4 on a uClibc target. The linux_valgrind error looks unrelated (due to the recent googleurl changes?), and maruel tells me that the linux_precise and linux_rel_precise trybots no longer exist (which is why those jobs are permanently pending). This this look good to you?
@jln: *ping* :)
Sorry, this slipped through. I'm always wary of having #ifdefs for configuration that aren't tested on the main waterfall. What would you think of either: - Drop xstat completely if we think that stat / stat64 is enough. Adam, do you remember if we had seen anything else than these two ? - Maybe safer: have your stat / stat64 shim always compile, even if we have xstat. It shouldn't hurt and at least it'll get tested. What do you think ? https://chromiumcodereview.appspot.com/17066002/diff/16001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/16001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:197: struct stat *buf) __asm__ ("stat"); Nit: one less space. https://chromiumcodereview.appspot.com/17066002/diff/16001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:213: struct stat64 *buf) __asm__ ("stat64"); Nit: one less space.
https://chromiumcodereview.appspot.com/17066002/diff/16001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/16001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:197: struct stat *buf) __asm__ ("stat"); On 2013/06/24 21:53:01, Julien Tinnes wrote: > Nit: one less space. Done. https://chromiumcodereview.appspot.com/17066002/diff/16001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:213: struct stat64 *buf) __asm__ ("stat64"); On 2013/06/24 21:53:01, Julien Tinnes wrote: > Nit: one less space. Done.
On 2013/06/24 21:53:01, Julien Tinnes wrote:
> Sorry, this slipped through.
>
> I'm always wary of having #ifdefs for configuration that aren't tested on the
> main waterfall.
>
> What would you think of either:
>
> - Drop xstat completely if we think that stat / stat64 is enough. Adam, do you
> remember if we had seen anything else than these two ?
That won't work for glibc, which at least in version 2.15 doesn't export
stat/stat64 (they seem to be inlined into __xstat/__xstat64).
$ readelf -a libc.so.6 | grep stat@
65: 000000000007b340 16 FUNC GLOBAL DEFAULT 12
_IO_file_stat@@GLIBC_2.2.5
452: 00000000000e62a0 69 FUNC GLOBAL DEFAULT 12 __lxstat@@GLIBC_2.2.5
1466: 00000000000e6250 69 FUNC GLOBAL DEFAULT 12 __fxstat@@GLIBC_2.2.5
1513: 00000000000f20d0 69 FUNC GLOBAL DEFAULT 12 ustat@@GLIBC_2.2.5
1929: 00000000000e6200 69 FUNC GLOBAL DEFAULT 12 __xstat@@GLIBC_2.2.5
> - Maybe safer: have your stat / stat64 shim always compile, even if we have
> xstat. It shouldn't hurt and at least it'll get tested.
>
> What do you think ?
This would work, with the following downsides (not showstoppers):
* two extra dlsym calls, possibly slow since two of the four calls will be for
symbols that don't exist
* it would require a tiny bit more logic to determine the fatal error situations
(if neither __xstat nor stat are found, or neither __xstat64 or stat64 are
found)
Personally I think this level of ifdeffing is not so bad- it's localised to a
single file (though there is some nesting with ADDRESS_SANITIZER). But if you
prefer this second option to ifdefs, I will make the changes.
On 2013/06/24 22:28:56, Mostyn Bramley-Moore wrote:
> On 2013/06/24 21:53:01, Julien Tinnes wrote:
> > Sorry, this slipped through.
> >
> > I'm always wary of having #ifdefs for configuration that aren't tested on
the
> > main waterfall.
> >
> > What would you think of either:
> >
> > - Drop xstat completely if we think that stat / stat64 is enough. Adam, do
you
> > remember if we had seen anything else than these two ?
>
> That won't work for glibc, which at least in version 2.15 doesn't export
> stat/stat64 (they seem to be inlined into __xstat/__xstat64).
>
> $ readelf -a libc.so.6 | grep stat@
> 65: 000000000007b340 16 FUNC GLOBAL DEFAULT 12
> mailto:_IO_file_stat@@GLIBC_2.2.5
> 452: 00000000000e62a0 69 FUNC GLOBAL DEFAULT 12
mailto:__lxstat@@GLIBC_2.2.5
> 1466: 00000000000e6250 69 FUNC GLOBAL DEFAULT 12
mailto:__fxstat@@GLIBC_2.2.5
> 1513: 00000000000f20d0 69 FUNC GLOBAL DEFAULT 12
mailto:ustat@@GLIBC_2.2.5
> 1929: 00000000000e6200 69 FUNC GLOBAL DEFAULT 12
mailto:__xstat@@GLIBC_2.2.5
>
> > - Maybe safer: have your stat / stat64 shim always compile, even if we have
> > xstat. It shouldn't hurt and at least it'll get tested.
> >
> > What do you think ?
>
> This would work, with the following downsides (not showstoppers):
> * two extra dlsym calls, possibly slow since two of the four calls will be for
> symbols that don't exist
> * it would require a tiny bit more logic to determine the fatal error
situations
> (if neither __xstat nor stat are found, or neither __xstat64 or stat64 are
> found)
>
> Personally I think this level of ifdeffing is not so bad- it's localised to a
> single file (though there is some nesting with ADDRESS_SANITIZER). But if you
> prefer this second option to ifdefs, I will make the changes.
Alright, let's do it! We had far too many issues of fringe #ifdef breaking and I
would really prefer if we avoided these.
What do you think of something such as:
1. static inline IsUsingXstat() {
#if defined()
return true;
#else
return false;
#endif // ...
}
2. Let all the rest compile every time (remove #if defined(HAVE_XSTAT)), but
have a comment explaining that xstat will be used only in certain configurations
and stat in others.
3. Before the relevant LOG(FATAL), add "&& (!)IsUsingXstat()"
4. Make sure that InitLibcFileIOFunctions() is called only once per Zygote
instance (i.e. ~per browser instance) by calling it from
InitLibcUrandomOverrides() explicitly (even before base::GetUrandomFD().
(InitLibcUrandomOverrides() is called from the Zygote early one, before it
starts forking).
Let me know what you think.
Sounds OK. I will try it out on a couple of different libcs tomorrow morning (it's getting late here).
@jln: How does patchset 6 look? * I didn't need to create IsUsingXstat, instead we can just check if the function pointers are non-NULL (and avoid another ifdef). * I had to dlsym two more functions than I mentioned earlier: __fxstat & __fxstat64. * Calling InitLibcFileIOFunctions from InitLibcUrandomOverrides didn't work, since stat is called before ZygoteMain. Do you mind if I handle this (removing the pthread_once calls) in a later code review?
> @jln: How does patchset 6 look? I'm worried about the complexity. Untested code = code that will rot and not work, so the logic must be kept a bit more simple. For instance I think __xstat and __fxstat are tied and would always exist or not exist at the same time. > * I didn't need to create IsUsingXstat, instead we can just check if the > function pointers are non-NULL (and avoid another ifdef). > * I had to dlsym two more functions than I mentioned earlier: __fxstat & > __fxstat64. Ohh, I see. That's because now that you don't have the #ifdef there is a compile-time dependency for __fxstat? I didn't forsee that. This may be a perfectly valid reason to keep a #ifdef around. But I'm not sold either way. Maybe it's ok to dynamically look for fxstat as you do, but we should check that fxstat are either both there or both not there? This should greatly simplify the logic. > * Calling InitLibcFileIOFunctions from InitLibcUrandomOverrides didn't work, > since stat is called before ZygoteMain. Do you mind if I handle this (removing > the pthread_once calls) in a later code review? Sorry I hadn't been clear. What I meant was to add a pthread_once(guard, InitLibcFileIOFunctions) in InitLibcUrandomOverrides. That way, we're guaranteed that it'll be initialized (even though in pratice it already is). https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:53: // Make sure /dev/urandom is open. Calling InitLibcFileIOFunctions() here would guarantee that it's initialized. Sure, in pratice GetUrandomFD() will stat() and something else will probably stat() before, so this call will be a no-op in pratice, but the guarantee would be here. Then it's much easier to assert, for instance in the Zygote, that InitLibcFileIOFunctions() will only be called one per browser instance (vs once per renderer for instance). So let's add: CHECK_EQ(0, pthread_once(&g_libc_file_io_funcs_guard, InitLibcFileIOFunctions)); https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:59: // find the libc's real fopen* and *stat* functions Nit: comment above the function, start with a capital, end with a dot. https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:62: // call it repeatedly with pthread_once() - ZygoteMain is not early enough. In practice it'll only be called once, right? If pthread_once() isn't readable enough, feel free to add a wrapper InitLibcFileIOFunctionsIfNecessary() that does the ptrace_once() call instead. Also document above InitLibcFileIOFunctions() that the function is supposed to be guarded by g_libc_file_io_funcs_guard. https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:105: // we can map them to fstat and fstat64. This makes everything way too complicated. In what configuration is xstat found, but not fxstat ? https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:172: return result; See my above comment, the logic becomes way too complicated.
https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:62: // call it repeatedly with pthread_once() - ZygoteMain is not early enough. On 2013/06/25 23:00:03, Julien Tinnes wrote: > In practice it'll only be called once, right? > > If pthread_once() isn't readable enough, feel free to add a wrapper > InitLibcFileIOFunctionsIfNecessary() that does the ptrace_once() call instead. > > Also document above InitLibcFileIOFunctions() that the function is supposed to > be guarded by g_libc_file_io_funcs_guard. This function is only called once, what I was trying to say is that pthread_once is called multiple times. I thought you wanted this reorganised to avoid the need for pthread_once... I'll play around with your suggestions and see if I can find something that works. https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:105: // we can map them to fstat and fstat64. On 2013/06/25 23:00:03, Julien Tinnes wrote: > This makes everything way too complicated. > > In what configuration is xstat found, but not fxstat ? In practice, probably none. As mentioned in my comment below, if we use fstat/fstat64 instead we would simplify things quite a bit. https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:172: return result; On 2013/06/25 23:00:03, Julien Tinnes wrote: > See my above comment, the logic becomes way too complicated. It should be safe to simply use fstat directly here instead of a dlsym'ed __fxstat (and similarly for the *64 versions). This would remove two dlsyms and these extra if cases. Would this be OK with you?
I replaced the __fxstat* code with fstat* in patchset 7. https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:59: // find the libc's real fopen* and *stat* functions On 2013/06/25 23:00:03, Julien Tinnes wrote: > Nit: comment above the function, start with a capital, end with a dot. Done.
Sorry, this thing is really super hairy and I want to be extremely careful to not create very subtle and hard to track bugs. I don't think we should be loosing "version". The bugs are horribly subtle, when a third party library (such as a binary GPU driver library) links to a different version of libc. To solve your linker issue with fxstat and friends, maybe just #ifdef that part with HAVE_XSTAT ? The glibc version would have all the code compiled-in, the non xstat version will have a subset. That way the code will still not rot. https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/33001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:62: // call it repeatedly with pthread_once() - ZygoteMain is not early enough. On 2013/06/25 23:40:41, Mostyn Bramley-Moore wrote: > On 2013/06/25 23:00:03, Julien Tinnes wrote: > > In practice it'll only be called once, right? > > > > If pthread_once() isn't readable enough, feel free to add a wrapper > > InitLibcFileIOFunctionsIfNecessary() that does the ptrace_once() call instead. > > > > Also document above InitLibcFileIOFunctions() that the function is supposed to > > be guarded by g_libc_file_io_funcs_guard. > > This function is only called once, what I was trying to say is that pthread_once > is called multiple times. I thought you wanted this reorganised to avoid the > need for pthread_once... I'll play around with your suggestions and see if I > can find something that works. Yeah, I'm not worried about mutiple calls to pthread_once, that's necessary. I just think we should guarantee that InitLibcUrandomOverrides() calls InitLibcFileIOFunctions(). https://chromiumcodereview.appspot.com/17066002/diff/40001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/40001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:156: int result = fstat(base::GetUrandomFD(), buf); Loosing "version" here, may lead into very subtle bugs, we should be extremely careful.
> Sorry, this thing is really super hairy and I want to be extremely careful to > not create very subtle and hard to track bugs. No problem. > I don't think we should be loosing "version". The bugs are horribly subtle, when > a third party library (such as a binary GPU driver library) links to a different > version of libc. Reverted these changes. > To solve your linker issue with fxstat and friends, maybe > just #ifdef that part > with HAVE_XSTAT ? > > The glibc version would have all the code compiled-in, the non xstat version > will have a subset. That way the code will still not rot. Done. I also changed the fatality of not finding various symbols based on HAVE_XSTAT: * If xstat is available, not being able to dlsym __xstat* is fatal but not stat* is not. * If xstat is not available, don't try to dlsym __xstat* and not being able to dlsym stat* is fatal. https://codereview.chromium.org/17066002/diff/33001/sandbox/linux/services/li... File sandbox/linux/services/libc_urandom_override.cc (right): https://codereview.chromium.org/17066002/diff/33001/sandbox/linux/services/li... sandbox/linux/services/libc_urandom_override.cc:53: // Make sure /dev/urandom is open. On 2013/06/25 23:00:03, Julien Tinnes wrote: > Calling InitLibcFileIOFunctions() here would guarantee that it's initialized. > Sure, in pratice GetUrandomFD() will stat() and something else will probably > stat() before, so this call will be a no-op in pratice, but the guarantee would > be here. > > Then it's much easier to assert, for instance in the Zygote, that > InitLibcFileIOFunctions() will only be called one per browser instance (vs once > per renderer for instance). > > So let's add: > > CHECK_EQ(0, pthread_once(&g_libc_file_io_funcs_guard, > InitLibcFileIOFunctions)); Done. https://codereview.chromium.org/17066002/diff/33001/sandbox/linux/services/li... sandbox/linux/services/libc_urandom_override.cc:62: // call it repeatedly with pthread_once() - ZygoteMain is not early enough. On 2013/06/26 01:57:21, Julien Tinnes wrote: > On 2013/06/25 23:40:41, Mostyn Bramley-Moore wrote: > > On 2013/06/25 23:00:03, Julien Tinnes wrote: > > > In practice it'll only be called once, right? > > > > > > If pthread_once() isn't readable enough, feel free to add a wrapper > > > InitLibcFileIOFunctionsIfNecessary() that does the ptrace_once() call > instead. > > > > > > Also document above InitLibcFileIOFunctions() that the function is supposed > to > > > be guarded by g_libc_file_io_funcs_guard. > > > > This function is only called once, what I was trying to say is that > pthread_once > > is called multiple times. I thought you wanted this reorganised to avoid the > > need for pthread_once... I'll play around with your suggestions and see if I > > can find something that works. > > Yeah, I'm not worried about mutiple calls to pthread_once, that's necessary. I > just think we should guarantee that InitLibcUrandomOverrides() calls > InitLibcFileIOFunctions(). Done in patchset 11. https://codereview.chromium.org/17066002/diff/33001/sandbox/linux/services/li... sandbox/linux/services/libc_urandom_override.cc:105: // we can map them to fstat and fstat64. On 2013/06/25 23:40:41, Mostyn Bramley-Moore wrote: > On 2013/06/25 23:00:03, Julien Tinnes wrote: > > This makes everything way too complicated. > > > > In what configuration is xstat found, but not fxstat ? > > In practice, probably none. As mentioned in my comment below, if we use > fstat/fstat64 instead we would simplify things quite a bit. Done (removed the fstat* dlsym's now). https://codereview.chromium.org/17066002/diff/33001/sandbox/linux/services/li... sandbox/linux/services/libc_urandom_override.cc:172: return result; On 2013/06/25 23:40:41, Mostyn Bramley-Moore wrote: > On 2013/06/25 23:00:03, Julien Tinnes wrote: > > See my above comment, the logic becomes way too complicated. > > It should be safe to simply use fstat directly here instead of a dlsym'ed > __fxstat (and similarly for the *64 versions). This would remove two dlsyms and > these extra if cases. Would this be OK with you? Done (reverted). https://codereview.chromium.org/17066002/diff/40001/sandbox/linux/services/li... File sandbox/linux/services/libc_urandom_override.cc (right): https://codereview.chromium.org/17066002/diff/40001/sandbox/linux/services/li... sandbox/linux/services/libc_urandom_override.cc:156: int result = fstat(base::GetUrandomFD(), buf); On 2013/06/26 01:57:21, Julien Tinnes wrote: > Loosing "version" here, may lead into very subtle bugs, we should be extremely > careful. Done (reverted).
Mostyn, Is this being done solely for the case of NSS? If so, how receptive are you to just including the patches that have been made to upstream NSS to simplify this - including simplifying the stat code? Would that address your needs without the crazy overrides?
On 2013/06/26 17:28:33, Ryan Sleevi wrote: > Mostyn, > > Is this being done solely for the case of NSS? If so, how receptive are you to > just including the patches that have been made to upstream NSS to simplify this > - including simplifying the stat code? Would that address your needs without the > crazy overrides? Yes, as far as I know, these stat family overrides are only used for NSS (actually anything that tries to open /dev/urandom). Do you have a pointer to the NSS patches (it will take me a while to download the source)?
On 2013/06/26 17:56:58, Mostyn Bramley-Moore wrote: > On 2013/06/26 17:28:33, Ryan Sleevi wrote: > > Mostyn, > > > > Is this being done solely for the case of NSS? If so, how receptive are you to > > just including the patches that have been made to upstream NSS to simplify > this > > - including simplifying the stat code? Would that address your needs without > the > > crazy overrides? > > Yes, as far as I know, these stat family overrides are only used for NSS > (actually anything that tries to open /dev/urandom). > > Do you have a pointer to the NSS patches (it will take me a while to download > the source)? Ah, it looks like I was mis-remembering, in that we opted to poke open the OS X sandbox a little more, but https://codereview.chromium.org/15990009/#ps1 explains where the call to stat is, and what the effects of being unable to stat would be. From the NSS developer call last Thursday, there's no objection/concern about the lack of stat() in the entropy gathering, as long as the actual read succeeds (and the Chrome patches are designed to make it abort() if not), but you may find this a much easier approach than attempting to hook stat.
Almost there. You can either RAW_CHECK as indicated, or feel free to just #else the stat case as in your original CL. I hadn't seen the static linker nor the version issues, and I'm not anymore convinced that this code shouldn't just not be compiled at all on glibc :) https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. You can, but don't need to update this anymore. https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:23: #if HAVE_XSTAT #if defined(HAVE_XSTAT) https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:25: #define XSTAT_VERSION 3 We really shouldn't hardcode this. This would lead to very subtle bugs, and I don't think anyone wants to go back to this code... ever :) https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:49: #if HAVE_XSTAT #if defined(HAVE_XSTAT) https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:89: #if HAVE_XSTAT #if defined(HAVE_XSTAT) https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:206: } do you want to use the } else { construct as above, just for consistency ? https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:211: #if HAVE_XSTAT #if defined(HAVE_XSTAT) https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:212: if (!g_libc_stat) { So, if I'm not mistaken, since this stat is inlined on glibc with xstat, this should never be called in practice, right? Let's just RAW_CHECK(false, "stat called on xstat libc") with the proper comment ? The comment should explain that we don't expect this to ever run on glibc because of xstat, but that it's not commented out to prevent the code from rotting. https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:236: return g_libc_xstat64(XSTAT_VERSION, path, buf); Same remark here of course.
On 2013/06/26 18:03:01, Ryan Sleevi wrote: > On 2013/06/26 17:56:58, Mostyn Bramley-Moore wrote: > > On 2013/06/26 17:28:33, Ryan Sleevi wrote: > > > Mostyn, > > > > > > Is this being done solely for the case of NSS? If so, how receptive are you > to > > > just including the patches that have been made to upstream NSS to simplify > > this > > > - including simplifying the stat code? Would that address your needs without > > the > > > crazy overrides? > > > > Yes, as far as I know, these stat family overrides are only used for NSS > > (actually anything that tries to open /dev/urandom). > > > > Do you have a pointer to the NSS patches (it will take me a while to download > > the source)? > > Ah, it looks like I was mis-remembering, in that we opted to poke open the OS X > sandbox a little more, but https://codereview.chromium.org/15990009/#ps1 > explains where the call to stat is, and what the effects of being unable to stat > would be. > > From the NSS developer call last Thursday, there's no objection/concern about > the lack of stat() in the entropy gathering, as long as the actual read succeeds > (and the Chrome patches are designed to make it abort() if not), but you may > find this a much easier approach than attempting to hook stat. Ryan: this has been a burden for many years now. Could you add something along the lines of "PresandboxInitialization()" in NSS that would save a fd to /dev/urandom and anything else it needs? We'd just call this and call it a day. I realize it'll take some time to trickle down, but I think it's really worth it.
On 2013/06/26 23:49:47, Julien Tinnes wrote: > Almost there. > > You can either RAW_CHECK as indicated, or feel free to just #else the stat case > as in your original CL. > > I hadn't seen the static linker nor the version issues, and I'm not anymore > convinced that this code shouldn't just not be compiled at all on glibc :) I think reverting back to something similar to patchset 5 might be the simplest safe solution. The non-glibc case won't be tested in the official waterfall, but we will be testing it daily and can submit fixes if things break. https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... File sandbox/linux/services/libc_urandom_override.cc (right): https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/06/26 23:49:47, Julien Tinnes wrote: > You can, but don't need to update this anymore. I will revert this if you prefer? https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:23: #if HAVE_XSTAT On 2013/06/26 23:49:47, Julien Tinnes wrote: > #if defined(HAVE_XSTAT) I would prefer to use the value of HAVE_XSTAT instead of the fact that it's defined, so it is possible to easily override this later. eg you could add -DHAVE_XSTAT=0 to the compiler flags if some version of glibc appears that does not support xstat. It's not very likely, so I won't mind changing back to check defined(HAVE_XSTAT), but I think the flexibility would be nice to have. https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:25: #define XSTAT_VERSION 3 On 2013/06/26 23:49:47, Julien Tinnes wrote: > We really shouldn't hardcode this. > > This would lead to very subtle bugs, and I don't think anyone wants to go back > to this code... ever :) This is only used when we are overriding stat/stat64, where we don't have an xstat version specified by the caller. The only other options I can see to hardcoding it are ifdefing out the stat override (like in patchset 5), or removing this "call xstat instead" fallback. I don't like the latter, because then we have done something possibly unsafe: provided a stat function that has a chance of failing due to not being able to find the real stat implementation (possibly because it doesn't exist, and headers are supposed to convert calling code into __xstat). https://chromiumcodereview.appspot.com/17066002/diff/54001/sandbox/linux/serv... sandbox/linux/services/libc_urandom_override.cc:212: if (!g_libc_stat) { On 2013/06/26 23:49:47, Julien Tinnes wrote: > So, if I'm not mistaken, since this stat is inlined on glibc with xstat, this > should never be called in practice, right? Correct- this whole function should never be called on glibc, since the header files convert stat* calls to __xstat*. The only way I think it can be called if code is built and linked against two different versions of glibc (unlikely but not impossible). But you asked for this code to be compiled even on glibc to avoid ifdefs, and therefore we assume a responsibility to handle this call as best we can in all situations, to avoid obscure failures. > Let's just RAW_CHECK(false, "stat called on xstat libc") with the proper comment > ? > > The comment should explain that we don't expect this to ever run on glibc > because of xstat, but that it's not commented out to prevent the code from > rotting. RAW_CHECK doesn't take a message argument, is it OK if I leave a source code comment instead?
I reverted back to two distinct cases in patchset 14, IMO this is simpler and easier to maintain. We will continue to test the case where xstat is not available in our own test infrastructure on a daily basis, and submit patches as required to keep this from bitrotting. We still need to agree on the following: * Should I leave the copyright date untouched? * Should we keep #if HAVE_XSTAT or revert to #if defined(HAVE_XSTAT) ?
On 2013/06/27 01:45:14, Mostyn Bramley-Moore wrote: > I reverted back to two distinct cases in patchset 14, IMO this is simpler and > easier to maintain. We will continue to test the case where xstat is not > available in our own test infrastructure on a daily basis, and submit patches as > required to keep this from bitrotting. Ok, lgtm! > We still need to agree on the following: > * Should I leave the copyright date untouched? We don't update the date anymore: http://www.chromium.org/developers/coding-style#TOC-File-headers > * Should we keep #if HAVE_XSTAT or revert to #if defined(HAVE_XSTAT) ? If it makes things easier for you to be able to pass the value as a compiler command line, feel free to keep it as-is.
> > We still need to agree on the following: > > * Should I leave the copyright date untouched? > > We don't update the date anymore: > http://www.chromium.org/developers/coding-style#TOC-File-headers OK, reverted. > > * Should we keep #if HAVE_XSTAT or revert to #if defined(HAVE_XSTAT) ? > > If it makes things easier for you to be able to pass the value as a compiler > command line, feel free to keep it as-is. I'll leave it as is then. When I get in to the office later this morning I will confirm that this works on my uClibc device, then add to CQ.
> When I get in to the office later this morning I will confirm that this works on > my uClibc device, then add to CQ. Looks good.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/17066002/45002
Message was sent while issue was closed.
Change committed as 208924 |
