From 0ecdec7a0922f5e99569c558fd3c6440b826e583 Mon Sep 17 00:00:00 2001 From: Mark Salyzyn Date: Tue, 1 Mar 2016 14:59:32 -0800 Subject: [PATCH 1/2] logd: clarify release_Locked() for static analyzer release_Locked() is called with a reference count and threadRunning, the static analyzer can not tell this and estimates that a call to delete this will occur. So let us invent a new call release_nodelete_Locked() to ensure it is clear we will not be arranging a delete this in the context of this code path. The delete this will follow in the immediate codepath in this function after threadRunning is cleared, and decRef_Locked() is called. Change will also remove any developer FUD regarding release_Locked() usage at this location. SideEffects: None Bug: 27434831 Change-Id: I91b060b2dadc72cc449fa381c934afb577bee037 --- logd/LogTimes.cpp | 2 +- logd/LogTimes.h | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/logd/LogTimes.cpp b/logd/LogTimes.cpp index a4b96d33d..2a04880e4 100644 --- a/logd/LogTimes.cpp +++ b/logd/LogTimes.cpp @@ -90,7 +90,7 @@ void LogTimeEntry::threadStop(void *obj) { while(it != times.end()) { if (*it == me) { times.erase(it); - me->release_Locked(); + me->release_nodelete_Locked(); break; } it++; diff --git a/logd/LogTimes.h b/logd/LogTimes.h index f5969df35..b66ff9e59 100644 --- a/logd/LogTimes.h +++ b/logd/LogTimes.h @@ -75,7 +75,13 @@ public: void triggerSkip_Locked(log_id_t id, unsigned int skip) { skipAhead[id] = skip; } void cleanSkip_Locked(void); - // Called after LogTimeEntry removed from list, lock implicitly held + // These called after LogTimeEntry removed from list, lock implicitly held + void release_nodelete_Locked(void) { + mRelease = true; + pthread_cond_signal(&threadTriggeredCondition); + // assumes caller code path will call decRef_Locked() + } + void release_Locked(void) { mRelease = true; pthread_cond_signal(&threadTriggeredCondition); From bf7d0b887561929d22c8e2efa5d86cb0d7c50b51 Mon Sep 17 00:00:00 2001 From: Mark Salyzyn Date: Wed, 2 Mar 2016 07:51:48 -0800 Subject: [PATCH 2/2] logd: check return values The setgid() and setuid() call failure in logd.daemon thread do not block overall functionality, so clearly tell static analyzer and developers that we do not care to check their return values. SideEffects: None Bug: 27434072 Change-Id: I6fdc87e8311ebc0173716080bbd72c86b3f00f78 --- logd/main.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/logd/main.cpp b/logd/main.cpp index f4d746416..3095f7f9d 100644 --- a/logd/main.cpp +++ b/logd/main.cpp @@ -165,7 +165,7 @@ bool property_get_bool(const char *key, int flag) { char newkey[PROPERTY_KEY_MAX]; snprintf(newkey, sizeof(newkey), "ro.%s", key); property_get(newkey, property, ""); - // persist properties set by /data require innoculation with + // persist properties set by /data require inoculation with // logd-reinit. They may be set in init.rc early and function, but // otherwise are defunct unless reset. Do not rely on persist // properties for startup-only keys unless you are willing to restart @@ -265,8 +265,11 @@ static void *reinit_thread_start(void * /*obj*/) { set_sched_policy(0, SP_BACKGROUND); setpriority(PRIO_PROCESS, 0, ANDROID_PRIORITY_BACKGROUND); - setgid(AID_SYSTEM); - setuid(AID_SYSTEM); + // If we are AID_ROOT, we should drop to AID_SYSTEM, if we are anything + // else, we have even lesser privileges and accept our fate. Not worth + // checking for error returns setting this thread's privileges. + (void)setgid(AID_SYSTEM); + (void)setuid(AID_SYSTEM); while (reinit_running && !sem_wait(&reinit) && reinit_running) {