From c8bae05f3ff9f1c736f7be70fa17d02795d748bb Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 4 Dec 2015 17:47:20 -0800 Subject: [PATCH] Revert "Remove __sinit and __sdidinit." This reverts commit 4371961e00ad83fca033992c8a19c7d262fe6f84. This broke booting; ueventd crashes with a null pointer dereference somewhere in __sfp (but the kernel doesn't unwind, so I don't know what was calling __sfp). Change-Id: I65375fdfdf1d339a06558b4057b580cacd6324e2 --- libc/bionic/flockfile.cpp | 12 +++++++++++ libc/bionic/libc_init_common.cpp | 8 ++++---- libc/bionic/ndk_cruft.cpp | 7 ------- libc/stdio/findfp.c | 44 ++++++++++++++++++++++++++++++++++------ libc/stdio/local.h | 7 +++---- libc/stdio/refill.c | 5 +++++ 6 files changed, 62 insertions(+), 21 deletions(-) diff --git a/libc/bionic/flockfile.cpp b/libc/bionic/flockfile.cpp index db5382859..db688016b 100644 --- a/libc/bionic/flockfile.cpp +++ b/libc/bionic/flockfile.cpp @@ -36,12 +36,20 @@ // struct __sfileext (see fileext.h). void flockfile(FILE* fp) { + if (!__sdidinit) { + __sinit(); + } + if (fp != nullptr) { pthread_mutex_lock(&_FLOCK(fp)); } } int ftrylockfile(FILE* fp) { + if (!__sdidinit) { + __sinit(); + } + // The specification for ftrylockfile() says it returns 0 on success, // or non-zero on error. So return an errno code directly on error. if (fp == nullptr) { @@ -52,6 +60,10 @@ int ftrylockfile(FILE* fp) { } void funlockfile(FILE* fp) { + if (!__sdidinit) { + __sinit(); + } + if (fp != nullptr) { pthread_mutex_unlock(&_FLOCK(fp)); } diff --git a/libc/bionic/libc_init_common.cpp b/libc/bionic/libc_init_common.cpp index 8f210ea7c..91e210e3e 100644 --- a/libc/bionic/libc_init_common.cpp +++ b/libc/bionic/libc_init_common.cpp @@ -54,7 +54,7 @@ extern "C" abort_msg_t** __abort_message_ptr; extern "C" int __system_properties_init(void); extern "C" int __set_tls(void* ptr); extern "C" int __set_tid_address(int* tid_address); -extern "C" int __libc_init_stdio(void); +extern "C" int __sinit(void); __LIBC_HIDDEN__ WriteProtected __libc_globals; @@ -134,9 +134,9 @@ void __libc_init_common(KernelArgumentBlock& args) { __pthread_internal_add(main_thread); __system_properties_init(); // Requires 'environ'. - - // Initialize stdio to avoid data races caused by BSD-style lazy initialization. - __libc_init_stdio(); + // Initialize stdio here to get rid of data races caused by lazy initialization. + // TODO: Remove other calls to __sinit(). + __sinit(); } __noreturn static void __early_abort(int line) { diff --git a/libc/bionic/ndk_cruft.cpp b/libc/bionic/ndk_cruft.cpp index 0af9d5bc2..d6b8e8f02 100644 --- a/libc/bionic/ndk_cruft.cpp +++ b/libc/bionic/ndk_cruft.cpp @@ -286,13 +286,6 @@ extern "C" int getdtablesize() { return r.rlim_cur; } -// A leaked BSD stdio implementation detail that's now a no-op. -// (GCC doesn't like 'extern "C"' on a definition.) -extern "C" { -void __sinit() {} -int __sdidinit = 1; -} - // Only used by ftime, which was removed from POSIX 2008. struct timeb { time_t time; diff --git a/libc/stdio/findfp.c b/libc/stdio/findfp.c index 4054d5f3c..2696cfd25 100644 --- a/libc/stdio/findfp.c +++ b/libc/stdio/findfp.c @@ -44,6 +44,8 @@ #define ALIGNBYTES (sizeof(uintptr_t) - 1) #define ALIGN(p) (((uintptr_t)(p) + ALIGNBYTES) &~ ALIGNBYTES) +int __sdidinit; + #define NDYNAMIC 10 /* add ten more whenever necessary */ #define std(flags, file) \ @@ -112,6 +114,9 @@ __sfp(void) int n; struct glue *g; + if (!__sdidinit) + __sinit(); + _THREAD_PRIVATE_MUTEX_LOCK(__sfp_mutex); for (g = &__sglue; g != NULL; g = g->next) { for (fp = g->iobs, n = g->niobs; --n >= 0; fp++) @@ -144,21 +149,48 @@ found: return (fp); } -static void __stdio_cleanup(void) { +/* + * exit() and abort() call _cleanup() through the callback registered + * with __atexit_register_cleanup(), set whenever we open or buffer a + * file. This chicanery is done so that programs that do not use stdio + * need not link it all in. + * + * The name `_cleanup' is, alas, fairly well known outside stdio. + */ +void +_cleanup(void) +{ /* (void) _fwalk(fclose); */ (void) _fwalk(__sflush); /* `cheating' */ } -void __libc_init_stdio(void) { - // Initialize stdin/stdout/stderr (for the recursive mutex). http://b/18208568. +/* + * __sinit() is called whenever stdio's internal variables must be set up. + */ +void +__sinit(void) +{ + _THREAD_PRIVATE_MUTEX(__sinit_mutex); + + _THREAD_PRIVATE_MUTEX_LOCK(__sinit_mutex); + if (__sdidinit) { + /* bail out if caller lost the race */ + _THREAD_PRIVATE_MUTEX_UNLOCK(__sinit_mutex); + return; + } + + /* Initialize stdin/stdout/stderr (for the recursive mutex). http://b/18208568. */ for (size_t i = 0; i < 3; ++i) { _FILEEXT_SETUP(__sF+i, __sFext+i); } - // Initialize the pre-allocated (but initially unused) streams. + /* Initialize the pre-allocated (but initially unused) streams. */ for (size_t i = 0; i < FOPEN_MAX - 3; ++i) { _FILEEXT_SETUP(usual+i, usualext+i); } - // Make sure we clean up on exit. - __atexit_register_cleanup(__stdio_cleanup); /* conservative */ + /* make sure we clean up on exit */ + __atexit_register_cleanup(_cleanup); /* conservative */ + __sdidinit = 1; + + _THREAD_PRIVATE_MUTEX_UNLOCK(__sinit_mutex); } diff --git a/libc/stdio/local.h b/libc/stdio/local.h index db3068df4..3ae7059da 100644 --- a/libc/stdio/local.h +++ b/libc/stdio/local.h @@ -153,8 +153,10 @@ __LIBC32_LEGACY_PUBLIC__ int __srefill(FILE*); __LIBC32_LEGACY_PUBLIC__ int __swsetup(FILE*); /* These were referenced by a couple of different pieces of middleware and the Crystax NDK. */ +__LIBC32_LEGACY_PUBLIC__ extern int __sdidinit; __LIBC32_LEGACY_PUBLIC__ int __sflags(const char*, int*); __LIBC32_LEGACY_PUBLIC__ FILE* __sfp(void); +__LIBC32_LEGACY_PUBLIC__ void __sinit(void); __LIBC32_LEGACY_PUBLIC__ void __smakebuf(FILE*); /* These are referenced by the Greed for Glory franchise. */ @@ -168,6 +170,7 @@ __LIBC32_LEGACY_PUBLIC__ int _fwalk(int (*)(FILE *)); #pragma GCC visibility push(hidden) int __sflush_locked(FILE *); +void _cleanup(void); int __swhatbuf(FILE *, size_t *, int *); wint_t __fgetwc_unlock(FILE *); wint_t __ungetwc(wint_t, FILE *); @@ -234,10 +237,6 @@ struct __suio; extern int __sfvwrite(FILE *, struct __suio *); wint_t __fputwc_unlock(wchar_t wc, FILE *fp); -/* Remove the if (!__sdidinit) __sinit() idiom from untouched upstream stdio code. */ -extern void __sinit(void); // Not actually implemented. -#define __sdidinit 1 - #pragma GCC visibility pop __END_DECLS diff --git a/libc/stdio/refill.c b/libc/stdio/refill.c index 5b0811f6a..e87c7b93e 100644 --- a/libc/stdio/refill.c +++ b/libc/stdio/refill.c @@ -51,6 +51,11 @@ lflush(FILE *fp) int __srefill(FILE *fp) { + + /* make sure stdio is set up */ + if (!__sdidinit) + __sinit(); + fp->_r = 0; /* largely a convenience for callers */ #if !defined(__ANDROID__) -- 2.11.0