Skip to content

GitLab

Switch branch/tag
  1. 08 Jun, 2016 1 commit
    • Shubham Ajmera's avatar
      Correct address fields in ByteBufferAsXBuffer classes · 10bac635
      Shubham Ajmera authored 
      The value of the address field should be equals to the first element of
the ByteBufferAsXBuffer class, previously, it was pointing to the first
element of the underlying directbytebuffer object.

Bug: 28964300
(cherry-picked from commit e0f383ff)
Change-Id: I6dfc98ba408c524580198ea168923981b23a0bc2
      10bac635
  3. 01 Jun, 2016 1 commit
  5. 31 May, 2016 1 commit
  7. 27 May, 2016 5 commits
    • Narayan Kamath's avatar
      FileDispatcherImpl: Untag sockets before preClose. · 9666717e
      Narayan Kamath authored 
      We shouldn't close them after preClose because the descriptor
will then describe a different file (/dev/null).

bug: 28994821

(cherry picked from commit 48689e680a503e82cbe1d5cc122f4bb628aa2fa2)

Change-Id: I3c3519cb3a54fdebf6d51520747735af01664696
      9666717e
    • Joachim Sauer's avatar
      Merge "Sync default timezone to ICU." into nyc-dev · 75fc6a28
      Joachim Sauer authored
      75fc6a28
    • Przemyslaw Szczepaniak's avatar
      Merge "Fix java.util.zip.ZipFile with OPEN_DELETE." into nyc-dev · 09913b03
      Przemyslaw Szczepaniak authored
      09913b03
    • Narayan Kamath's avatar
      Selector: Use poll based selector instead of an epoll based selector. · 4585ee7a
      Narayan Kamath authored 
      The OpenJDK epoll based selector suffers from a serious bug where it
can never successfully deregister keys from closed channels.

The root cause of this bug is the sequence of operations that occur when
a channel that's registered with a selector is closed :

(0) Application code calls Channel.close().

(1) The channel is "preClosed" - We dup2(2) /dev/null into the channel's
file descriptor and the channel is marked as closed at the Java level.

(2) All keys associated with the channel are cancelled. Cancels are
lazy, which means that the Selectors involved won't necessarily
deregister these keys until an ongoing call to select() (if any) returns
or until the next call to select() on that selector.

(3) Once all selectors associated with the channel deregister these
cancelled keys, the channel FD is properly closed (via close(2)). Note
that an arbitrary length of time might elapse between Step 0 and this step.
This isn't a resource leak because the channel's FD is now a reference
to "/dev/null".

THE PROBLEM :
-------------
The default Selector implementation on Linux 2.6 and higher uses epoll(7).
epoll can scale better than poll(2) because a lot of the state related
to the interest set (the set of descriptors we're polling on) is
maintained by the kernel. One of the side-effects of this design is that
callers must call into the kernel to make changes to the interest set
via epoll_ctl(7), for eg., by using EPOLL_CTL_ADD to add descriptors or
EPOLL_CTL_DEL to remove descriptors from the interest set. A call to
epoll_ctl with op = EPOLL_CTL_DEL is made when the selector attempts to
deregister an FD associated with a channel from the interest set (see
Step 2, above). These calls will *always fail* because the channel has
been preClosed (see Step 1). They fail because the kernel uses its own
internal file structure to maintain state, and rejects the command
because the descriptor we're passing in describes a different file
(/dev/null) that isn't selectable and isn't registered with the epoll
instance.

This is an issue in upstream OpenJDK as well and various select
implementations (such as netty) have hacks to work around it. Outside
of Android, things will work OK in most cases because the kernel has its
own internal cleanup logic to deregister files from epoll instances
whenever the last *non epoll* reference to the file has been closed -
and usually this happens at the point at which the dup2(2) from Step 1
is called. However, on Android, sockets tagged with the SocketTagger
will never hit this code path because the socket tagging implementation
(qtaguid) keeps a reference to the internal file until the socket
has been untagged. In cases where sockets are closed without being
untagged, the tagger keeps a reference to it until the process dies.

THE SOLUTION :
--------------
We switch over to using poll(2) instead of epoll(7). One of the
advantages of poll(2) is that there's less state maintained by the
kernel. We don't need to make a syscall (analogous to epoll_ctl)
whenever we want to remove an FD from the interest set; we merely
remove it from the list of FDs passed in the next time we call
through to poll. Poll is also slightly more efficient and less
overhead to set up when the number of FDs being polled is small
(which is the common case on Android).

We also need to make sure that all tagged sockets are untagged before
they're preclosed at the platform level. However, there's nothing we
can do about applications that abuse public api (android.net.TrafficStats).

ALTERNATE APPROACHES :
----------------------
For completeness, I'm listing a couple of other approaches that were
considered but discarded.

- Removing preClose: This has the disadvantage of increasing the amount
  of time (Delta between Step 0 and Step 3) a channel's descriptor is
  kept alive. This also opens up races in the rare case where a
  closed FD number is reused on a different thread while we have reads
  pending.

- A Synchronous call to EPOLL_CTL_DEL when a channel is removed: This is a
  non-starter because of the specified order of events in
  AbstractSelectableChannel; implCloseSelectableChannel must be called
  before all associated keys are cancelled.

bug: 28318596
Change-Id: I407b06b4b538e19823ac27a4c9ae4c608a01a2ea
      4585ee7a
    • Narayan Kamath's avatar
      OsTest: Explicitly delete temporary files. · 0c2a90d8
      Narayan Kamath authored 
      Don't rely on temporary directory cleanup.

bug: 28946760
Change-Id: I2cded2d0ffe6ea05008295460f8e16b09f8ad6f4
      0c2a90d8
  9. 26 May, 2016 3 commits
    • Przemyslaw Szczepaniak's avatar
      Fix java.util.zip.ZipFile with OPEN_DELETE. · a128ef7c
      Przemyslaw Szczepaniak authored 
      openJdk java.util.zip.ZipFile with OPEN_DELETE flag
uses unlink to remove the file just after it's opened.

This is causing trouble on /storage partition, we
can read/write unlinked file easily, but lseek seems
to fail with ENOENT.

This change introduces alternative implementation that
doesn't use unlink before closing the file.

Bug: 28901232
Bug: 28950284
Change-Id: I871a84e9a14bc1b4b9d5b0faa207579e27bcfc81
(cherry picked from commit ae6b1b85)
      a128ef7c
    • Joachim Sauer's avatar
      Sync default timezone to ICU. · f59264d3
      Joachim Sauer authored 
      When java.util.TimeZone.setDefault() is called (either by client code or
from ActivityThread.updateTimeZone due to ACTION_TIMEZONE_CHANGED) we
need to notify android.icu.util.TimeZone of this change, as it keeps a
cached android.icu.util.TimeZone object to represent that default value.

android.icu.util.TimeZone.setTimeZone would be the obvious candidate
here. Unfortunately that method was hidden to have a single consistent
way to set the timezone and tries to do some extra work that is
undesireable on Android.

To avoid potential loops of ICU setDefault calling JDK setDefault and
vice versa, we remove the ICU setDefault.

Bug: 28949992
Change-Id: I46a59551864cd2b780f72d28e01a0c39daf3aef5
      f59264d3
    • Neil Fuller's avatar
      Merge "Relax tests around HTTP header validation" into nyc-dev · 2276af34
      Neil Fuller authored
      2276af34
  11. 25 May, 2016 2 commits
    • Neil Fuller's avatar
      Relax tests around HTTP header validation · e62b4c58
      Neil Fuller authored 
      An associated change in OkHttp is relaxing the validation to
make it closer to the behavior in M. This may be temporary and
could be removed in a future release of Android.

Bug: 28867041
Bug: https://code.google.com/p/android/issues/detail?id=210205
Change-Id: If45f89578553e38ad455850f7f6bbecd4d51262e
      e62b4c58
    • Joachim Sauer's avatar
      Fix setCurrency resetting fraction digits. · 13659d2d
      Joachim Sauer authored 
      ICU DecimalFormat and java.text.DecimalFormat differ in their
definition of setCurrency in that the former is documented to leave the
minimum and maximum fraction digits untouched and the later updates it
to the default of the currency.

Since we implement java.text.DecimalFormat using the ICU DecimalFormat
we have to work around that difference.

Bug: 28893763
Change-Id: I0e7e19fe95f352e75cc9f53ad8a7739436de37d1
      13659d2d
  13. 24 May, 2016 16 commits
  15. 23 May, 2016 1 commit
    • Tobias Thierer's avatar
      Fix race condition in accept() on socket that is concurrently closed. · bcb10462
      Tobias Thierer authored 
      This CL is the result of long digging into the rabbit hole after
observing a flaky test. I've added a unit test for behavior at
the ServerSocket level, although the root cause of the bug sits
lower.

ServerSocket.accept() checks for a closed connection at the Java
level before descending into deeper (including native) levels.
The problem was that if the socket was closed() concurrently
*after* this initial check at the Java level, later native code
didn't deal with the closed state correctly and could block
indefinitely.

Specifically, a closed socket internally sets the value of its
fd field to -1. Native code reads that field via

      (*env)->GetIntField(env, fdObj, IO_fd_fdID);

and passes it to NET_Timeout, which then blocks. Other places
in the code read the same field and pass it to NET_Accept and
NET_Connect, which do not block when they get an fd value of -1.

There are 7 places that I found which call NET_Timeout, and they
all get their fd value from the Java field using the code snippet
above. NET_Timeout is documented to be a "Wrapper for
poll(s, timeout)". However, it was never a particularly thin
wrapper: unlike NET_Poll, NET_Timeout already has a bunch of
extra logic on top of the poll syscall, namely ignoring other
signals and adjusting the timeout. All of this suggests that
NET_Timeout is the right place to handle the case of fd AKA s
being < 0.

As discussed on the bug, observability in the sense of
ServerSocketConcurrentCloseTest failing was caused by:

  https://android.googlesource.com/platform/libcore/+/eeb3b74

It's therefore probably a good idea to investigate whether our
current mechanics for closing sockets are the right ones.
Establishing correctness of such an alternative change would
require very careful analysis, though. Therefore I went for
this fix instead, which is much easier to verify as safe and
corrects all observable issues that we currently know about.

Tested:
 - Ran the test many times from Android Studio (adjusted
   test's packageName). This is the only test that I also did
   on devices without the fix.
   - on build with the fix, the test passes 25/25 times.
   - on build without the fix, the test fails 12/15 times.
 - Also ran a couple of times from CTS (25181x pass, 19x skipped):
   make cts && cts-tradefed run cts -m CtsLibcoreTestCases \
     --test java.net.ServerSocketConcurrentCloseTest
 - Ran all of the libcore tests once through CTS
   (25181 tests pass, 19 tests not run):
   make cts && cts-tradefed run cts -m CtsLibcoreTestCases
 - make cts && cts-tradefed run cts -m CtsNetTestCases

Tests were run on a Nexus 6P. I previously verified that the
bug also occurs on Nexus 5X and Nexus 6 running N.

Bug: 27763633
Change-Id: I074acebe3d3c8a252bf5107bf45be9589d098c6a
      bcb10462
  17. 20 May, 2016 2 commits
  19. 19 May, 2016 2 commits
  21. 18 May, 2016 3 commits
  23. 17 May, 2016 3 commits