search

termux / termux-packages

A package build system for Termux.
https://termux.dev
Other
13.12k stars 3.01k forks source link

[Bug]: libandroid-posix-semaphore implementation issues #17471

Open FrankHB opened 1 year ago

FrankHB commented 1 year ago

Problem description

There are multiple issues found in the source code of the named semaphore implementation.

Since the source of the package is directly maintained in this repository without some upstream, issues are reported here. (Although I see most of the code vendored here has the upstream: musl.)

List of issues
(Minor) Bloated __strchrnul

https://github.com/termux/termux-packages/blob/0cb89e24e5d872af1d4cd84f452100a45d3696a0/packages/libandroid-posix-semaphore/semaphore.c#L49

__strchrnul is bloated. Looks like better inlined into the call site, since it is used only once. Extraction of the function into file scope does not make it more readable. The cast away of const is also a source of suspicous mess.

(Minor) Misleading comment

https://github.com/termux/termux-packages/blob/0cb89e24e5d872af1d4cd84f452100a45d3696a0/packages/libandroid-posix-semaphore/semaphore.c#L110-L112

The comment is misleading, because it is not that "necessary". Some other implementations (notably glibc) use different approach: cleanup later instead of preallocating the slot eagerly. Nevertheless, the current code is OK. This is at least simpler compared to glibc's approach which needs to set and recover errno around munmap on further failure path.

Handling of reference count before creation

https://github.com/termux/termux-packages/blob/0cb89e24e5d872af1d4cd84f452100a45d3696a0/packages/libandroid-posix-semaphore/semaphore.c#L114-L123

cnt += semtab[i].refcnt; likely leads to undefined behavior if it overflows. The comment later ("Avoid possibility of overflow later") is then nonsense in this case, because it has already overflow.

+= is just the WTF stuff, because it serves nothing to do with limitations from the spec. Should this actually be =?

Missing handling of EINTR for write

https://github.com/termux/termux-packages/blob/0cb89e24e5d872af1d4cd84f452100a45d3696a0/packages/libandroid-posix-semaphore/semaphore.c#L171-L175

The call to write may be interrupted. The upstream code does not use writealso does not handle it properly (although it is in a loop). OTOH, glibc has the logic by using TEMP_FAILURE_RETRY:

https://github.com/bminor/glibc/blob/4290aed05135ae4c0272006442d147f2155e70d7/sysdeps/pthread/sem_open.c#L163-L168

Missing handling of pthread calcellation

Both musl and glibc has logic to temporarily disable pthread cancellation. Nothing is here.

Is this just unsupported?

Missing translation of errno for sem_unlink

unlink may return EPERM, which is not allowed for sem_unlink.

Musl is also buggy here. OTOH, glibc will set errno appropriately.

Remarks

Many but not all of the issues are also in musl.

What steps will reproduce the bug?

Since the bugs are found by source code auditing, it just takes some time to direct to the source code. Then, read carefully.

What is the expected behavior?

See the discussions in the list of issues in problem description.

Test cases are not provided due to non-triviality.

System information

termux-info:

(I don't see it relavant here because the analysis is totally off the runtime, but OK...)

Termux Variables:
TERMUX_APK_RELEASE=GITHUB
TERMUX_APP_PACKAGE_MANAGER=apt
TERMUX_APP_PID=16280
TERMUX_IS_DEBUGGABLE_BUILD=1
TERMUX_MAIN_PACKAGE_FORMAT=debian
TERMUX_VERSION=0.117
Packages CPU architecture:
aarch64
Subscribed repositories:
# sources.list
deb https://mirror.sjtu.edu.cn/termux/termux-main/ stable main
# sources.list.d/game.list
deb https://packages.termux.org/apt/termux-games games stable
# sources.list.d/science.list
deb https://packages.termux.org/apt/termux-science science stable
# root-repo (sources.list.d/root.list)
deb https://mirror.sjtu.edu.cn/termux/termux-root/ root stable
Updatable packages:
All packages up to date
termux-tools version:
1.38.3
Android version:
13
Kernel build information:
Linux localhost 5.10.66-android12-9-00005-gf6e6376090be-ab8060604 #1 SMP PREEMPT Fri Jan 7 14:51:36 UTC 2022 aarch64 Android
Device manufacturer:
nubia
Device model:
NX709J
LD Variables:
LD_LIBRARY_PATH=/data/data/com.termux/files/home/sysroot/usr/lib:
LD_PRELOAD=/data/data/com.termux/files/usr/lib/libtermux-exec.so
licy183 commented 1 year ago

Thanks for your review, but I think almost all your reviews should be submitted to musl upstream.

  1. (Minor) Bloated __strchrnul

This function is copied from musl. https://elixir.bootlin.com/musl/latest/source/src/string/strchrnul.c#L10

Yeah, this function can be replaced by strchrnul because strchrnul is introduced since Android 7.

  1. (Minor) Misleading comment

  2. Handling of reference count before creation

These comments and codes are copied from musl libc without any change. So you'd better report this issue to musl upstream.

  1. Missing handling of EINTR for write

Upstream also use write without any protection. https://elixir.bootlin.com/musl/latest/source/src/thread/sem_open.c#L124

  1. Missing handling of pthread calcellation

All of the functions of pthread cancellation is not supported by Bionic libc (see https://android.googlesource.com/platform/bionic/+/master/docs/status.md).

FrankHB commented 1 year ago

Thanks for your review, but I think almost all your reviews should be submitted to musl upstream.

  1. (Minor) Bloated __strchrnul

This function is copied from musl. https://elixir.bootlin.com/musl/latest/source/src/string/strchrnul.c#L10

Yeah, this function can be replaced by strchrnul because strchrnul is introduced since Android 7.

I see. However, manually inlining this into the call site can be even clearer. (Or probably also better report it to musl?)

  1. (Minor) Misleading comment
  2. Handling of reference count before creation

These comments and codes are copied from musl libc without any change. So you'd better report this issue to musl upstream.

  1. Missing handling of EINTR for write

Upstream also use write without any protection. https://elixir.bootlin.com/musl/latest/source/src/thread/sem_open.c#L124

Thanks for confirmation. I'll report them to upstream.

  1. Missing handling of pthread calcellation

All of the functions of pthread cancellation is not supported by Bionic libc (see https://android.googlesource.com/platform/bionic/+/master/docs/status.md).

Glad to see this. (esp. "Unlikely to ever be implemented"... as a few things actually ease the life with Android.)

FrankHB commented 1 year ago

@richfelker For probably better contextual information, would you mind to continue the discussion here, or I should transfer the questions to musl mailing list?