If wake() fails to write to mWakeEventFd, the looper is
screwed. This is really a fatal, unrecoverable error,
so treat it as such.
This is possibly an explaination for the unexplained
random ANRs
Change-Id: I5af3e013493b475d8e902d8ee6340f331e5d86c8
This prevents two different kinds of client errors from causing
undetected memory corruption, and helps with the detection of others:
1. We no longer deallocate objects when the weak count goes to zero
and there have been no strong references. This otherwise causes
us to return a garbage object from a constructor if the constructor
allocates and deallocates a weak pointer to this. And we do know
that clients allocate such weak pointers in constructors and their
lifetime is hard to trace.
2. We abort if a RefBase object is explicitly destroyed while
the weak count is nonzero. Otherwise a subsequent decrement
would cause a write to potentially reallocated memory.
3. We check counter values returned by atomic decrements for
plausibility, and fail immediately if they are not plausible.
We unconditionally log any cases in which 1 changes behavior
from before. We abort in cases in which 2 changes behavior, since
those reflect clear bugs.
In case 1, a log message now indicates a possible leak. We have
not seen such a message in practice.
The third point introduces a small amount of overhead into the
reference count decrement path. But this should be negligible
compared to the actual decrement cost.
Add a test for promote/attemptIncStrong that tries to check for
both (1) above and concurrent operation of attemptIncStrong.
Add some additional warnings and explanations to the RefBase
documentation.
Bug: 30503444
Bug: 30292291
Bug: 30292538
Change-Id: Ida92b9a2e247f543a948a75d221fbc0038dea66c
Add basic interface documentation to RefBase.h.
Much, but not all, of this is cut-and-pasted from an email message
from Mathias Agopian. The rest is reconstructed from the code.
Delete some, now redundant, text from Refbase.cpp, and add a bit
more about the implementation strategy.
Some minor fixes to internal comments.
Bug: 30292291
Change-Id: I56518ae5553bc6de0cc2331778e7fcf2e6c4fd87
Since the equality operator '==' has higher precedence than the
assignment operator '=', we were assigning 'prev' to the result of
our comparison and not the result of mRefs.fetch_sub().
This means that 'prev' would only receive the values 0 or 1. In
the cases where fetch_sub() returned 0 or 1, we were happening to
get the correct value. But if fetch_sub() was greator than 1,
we would return to the user 0, instead of the previous reference
count.
We fix this by properly adding parentheses. We also adjust the
whitespace a little to hopefully make the groupings of the logic
easier to see.
Change-Id: Ib129798a7076854b9ca4f6385c42edbf4fb75e57
We refactor this mildly in the hopes of making this a little easier
to follow for future readers. We also go through the dealloc()
method so if we decide to invoke the SharedBuffer destructor in
the future, we only need to remember it in one place.
In addition, this is slightly more efficient in the multi-owner
case, since we no longer subtract to 0 and then (redundantly) set the
reference count explicitly to 0 as well.
Change-Id: Ifc773bd7900c89c36ac24904b2716f02cb57c095
Since the equality operator '==' has higher precedence than the
assignment operator '=', we were assigning 'prev' to the result of
our comparison and not the result of mRefs.fetch_sub().
This means that 'prev' would only receive the values 0 or 1. In
the cases where fetch_sub() returned 0 or 1, we were happening to
get the correct value. But if fetch_sub() was greator than 1,
we would return to the user 0, instead of the previous reference
count.
We fix this by properly adding parentheses. We also adjust the
whitespace a little to hopefully make the groupings of the logic
easier to see.
Change-Id: Ib129798a7076854b9ca4f6385c42edbf4fb75e57
String16(const char *utf8) now returns the empty string in case
a string ends halfway throw a utf8 character.
Bug: 29267949
Change-Id: I5223caa7d42f4582a982609a898a02043265c6d3
The compensating onLastStrongRef call could be made even when there
was no onIncStrongAttempted call to compensate for. This
happened in the OBJECT_LIFETIME_STRONG case when e.g. curCount
was initially zero, but was concurrently incremented by another
thread.
I believe the old code was also incorrect in the
curCount = INITIAL_STRONG_VALUE + 1 case,
which seems to be possible under unlikely conditions.
In that case, I believe the compensating call IS needed.
Thus the condition was also changed.
Bug: 30503444
Change-Id: I44bcbcbb1264e4b52b6d3750dc39b041c4140381
Inconsistent behaviour between utf16_to_utf8 and utf16_to_utf8_length
is causing a heap overflow.
Correcting the length computation and adding bound checks to the
conversion functions.
Test: ran libutils_tests
Bug: 29250543
Change-Id: I6115e3357141ed245c63c6eb25fc0fd0a9a7a2bb
(cherry picked from commit c4966a363e)
Add some basic tests for RefBase, as well as a more ambitious memory
ordering test.
Add a README.txt with instructions to run the tests.
Comment out a couple of BlobCache tests that failed consistently and
appeared to be incorrect. With that fix, I managed to run
libutils_tests successfully on device.
Bug: 28705989
Change-Id: I8ad29995097a149a0cc38615d6ed37117ec6cb5c
This matches what the Android.mk defined, and should temporarily fix
builds that were broken with:
system/core/libutils/Unicode.cpp:225:12: runtime error: unsigned integer
overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned
long')
Change-Id: I0363b42fc2d62dfd2d05649c9aa9ef0be573e20a
Add comment that SharedBuffer is deprecated.
Both aref and SharedBuffer had memory ordering bugs. Aref has no
clients.
SharedBuffer had several bugs, which are fixed here:
mRefs was declared neither volatile, not atomic, allowing the
compiler to, for example, reuse a stale previously loaded value.
It used the default android_atomic release memory ordering, which
is insufficient for reference count decrements.
It used an ordinary memory read in onlyOwner() to check whether
an object is safe to deallocate, without any attempt to ensure
memory ordering.
Comments claimed that SharedBuffer was exactly 16 bytes, but
this was neither checked, nor correct on 64-bit platforms.
This turns mRef into a std::atomic and removes the android_atomic
dependency.
Bug: 28826227
Change-Id: I39fa0b4f70ac0471b14ad274806fc4e0c0802e78
(cherry picked from commit 3e4c076ef2)
Add comment that SharedBuffer is deprecated.
Both aref and SharedBuffer had memory ordering bugs. Aref has no
clients.
SharedBuffer had several bugs, which are fixed here:
mRefs was declared neither volatile, not atomic, allowing the
compiler to, for example, reuse a stale previously loaded value.
It used the default android_atomic release memory ordering, which
is insufficient for reference count decrements.
It used an ordinary memory read in onlyOwner() to check whether
an object is safe to deallocate, without any attempt to ensure
memory ordering.
Comments claimed that SharedBuffer was exactly 16 bytes, but
this was neither checked, nor correct on 64-bit platforms.
This turns mRef into a std::atomic and removes the android_atomic
dependency.
Bug: 28826227
Change-Id: I39fa0b4f70ac0471b14ad274806fc4e0c0802e78
Convert to use std::atomic directly.
Consistently use relaxed ordering for increments, release ordering
for decrements, and an added acquire fence when the count goes to
zero.
Fix what looks like another race in attemptIncStrong:
It seems entirely possible that the final adjustment for
INITIAL_STRONG_VALUE would see e.g. INITIAL_STRONG_VALUE + 1,
since we could be running in the middle of another initial
increment.
Attempt to somewhat document what this actually does, and
what's expected from the client. Hide the documentation in
the .cpp file for now.
Remove a confusing redundant test in decWeak. OBJECT_LIFETIME_STRONG
and OBJECT_LIFETIME_WEAK are the only options, in spite of some
of the original comments.
It's conceivable that either of these issues has resulted in
actual crashes, though I would guess the probability is small.
It's hard enough to reason about this code without the bugs.
Bug: 28705989
Change-Id: I4107a56c3fc0fdb7ee17fc8a8f0dd7fb128af9d8
(cherry picked from commit e263e6c633)
Convert to use std::atomic directly.
Consistently use relaxed ordering for increments, release ordering
for decrements, and an added acquire fence when the count goes to
zero.
Fix what looks like another race in attemptIncStrong:
It seems entirely possible that the final adjustment for
INITIAL_STRONG_VALUE would see e.g. INITIAL_STRONG_VALUE + 1,
since we could be running in the middle of another initial
increment.
Attempt to somewhat document what this actually does, and
what's expected from the client. Hide the documentation in
the .cpp file for now.
Remove a confusing redundant test in decWeak. OBJECT_LIFETIME_STRONG
and OBJECT_LIFETIME_WEAK are the only options, in spite of some
of the original comments.
It's conceivable that either of these issues has resulted in
actual crashes, though I would guess the probability is small.
It's hard enough to reason about this code without the bugs.
Bug: 28705989
Change-Id: I4107a56c3fc0fdb7ee17fc8a8f0dd7fb128af9d8
strcmp needs a limit, otherwise it will compare the null terminator
with the next character in the haystack, which results in the compare
failing for all searches except where the needle is found at the very
end.
Bug: 28663748
Change-Id: I1939dc4037c2f2a75d617943b063d2d38a8c5e3a
am: 6d28bd81f5
* commit '6d28bd81f55236d1a82f00f8ac568ad61a03128d':
SystemClock: elapsedRealtimeNano() should use clock_gettime() on Linux
Change-Id: Id5ecad63fb6cd79cc7db641d992e9525bc2b8779
