[BUG] mbstowcs and wcstombs cannot get correct return value when called in the environment below api 21

[Satsukina]

Description

use mbstowcs to convert multibyte to widechar and use wcstombs to convert back. But these two functions' return value are always 0 and convert fail.

example 1:
char *buffer = "1234";
int length = mbstowcs(NULL, buffer, 0);

The return value length is 0, its expected value should be 4.

example 2:
wchar_t *buffer = L"1234";
int length = wcstombs(NULL, buffer, 0);

The return value length is 0, its expected value should be 4.

Environment Details

Not all of these will be relevant to every bug, but please provide as much
information as you can.

• NDK Version: 20.0.5594570
• Build system: ndk-build
• Host OS: Windows
• ABI: armeabi-v7a, x86
• NDK API level: 16
• Device API level: 17

[DanAlbert]

Yeah, the implementation back then ignores a line from POSIX:

If pwcs is a null pointer, mbstowcs() shall return the length required to convert the entire array regardless of the value of n

This is already fixed in new devices. The only way to fix it for old devices is to make it a part of libandroid_support so that it's statically linked into everything. This comes pretty close to what we explicitly decided against over in #456.

This was fixed as of L (https://android-review.googlesource.com/c/platform/bionic/+/92730).

@enh-google wdyt? The workaround here is trivial (just pass strlen(buffer) + 1 instead of 0), so I'm not sure if we should impose that cost on every app. If we wanted to help users avoid this we could even have a fortify check on these pre-L that causes an error if you call this with a null dest ptr and a 0 length.

[rprichard]

The pre-L libc functions I looked at seem to treat wchar_t as being one byte in size, whereas it's always 4 bytes in the NDK. e.g.:

• printf("%ls\n", L"hello") ==> prints "h" in JB (prints "hello" in L)
• printf("%ls\n", (const wchar_t*)"hello") ==> prints "hello" in JB (prints nothing or crashes in L)
• mbstowcs(NULL, "\xce\x91" "\xce\x91" "\xce\x91", 256) ==> returns 7 in JB (returns 3 in L)
• wcstombs(NULL, L"123456789", 256) ==> returns 2 in JB (returns 9 in L)

The last example shows another problem -- pre-L mbstowcs/wcstombs return the size of their input strings plus 1. (I guess they're counting the NUL terminator?)

[Satsukina]

@DanAlbert Thanks for your reply. But just like @rprichard said, function's return values are different between JB and L. And if i try to use mbstowcs to convert a Chinese string, the converted string is incorrect in JB.

[enh-google]

given how broken these are pre-L it does seem like we should either change the #ifs so you don't get them if you're targeting earlier, or we should add them to the support library. right now we don't even document that they're broken pre-L :-(

[DanAlbert]

(if someone else wants to take this we can probably move to r22, but I think I'm already being overly optimistic about the number of things I've planned for myself in r22)

[enh-google]

we talked about this internally last month but never wrote down what we said, so here's my attempt to recap...

it looks like we have two main choices here:

1. stop declaring these for pre-L in the headers (and libc.map.txt) because they're broken. quick and easy, but not super helpful to anyone, and might break libc++ (because we're still on an older version that doesn't cope gracefully with attempts to say using ::something_not_defined;).
2. add this to libandroid_support.

it seems like we decided against 2, but i'm no longer sure why, and coming to this "fresh" (i.e. a month later so i don't remember all the details), 2 sounds like the better idea? especially because we already went so far down that route. look at ndk/sources/android/support/Android.mk:

android_support_sources := \
    $(BIONIC_PATH)/libc/bionic/c32rtomb.cpp \
    $(BIONIC_PATH)/libc/bionic/locale.cpp \
    $(BIONIC_PATH)/libc/bionic/mbrtoc32.cpp \
    $(BIONIC_PATH)/libc/bionic/wchar.cpp \
    $(BIONIC_PATH)/libc/upstream-openbsd/lib/libc/locale/mbtowc.c \
    src/locale_support.cpp \
    src/swprintf.cpp \
    src/wcstox.cpp \

it's all wide char stuff at this point. so it seems weird to miss out a couple of extra functions? (especially when one of the still-broken functions is mbstowcs() and we already added the single-character equivalent mbtowc()?)

a quick look suggests that both bionic/libc/upstream-openbsd/lib/libc/locale/mbstowcs.c and bionic/libc/upstream-openbsd/lib/libc/locale/wcstombs.c seem fine to include in libandroid_support?

[DanAlbert]

Re-reading that thread, I think those were intended as two steps rather than alternatives. Agreed with all your reasoning :)

[enh-google]

oh, so you're thinking:

3. add this to libandroid_support, roughly as described under 2 above, and also modify the bionic headers so if you compile without libandroid_support you don't get these two (broken) functions?

[DanAlbert]

That seems to be our best option, yeah.

[rprichard]

Should we remove other broken wide char functions from headers and libc.map.txt? e.g. wcstol, wcstod, wprintf

It looks like wchar_t became 4 bytes starting with Gingerbread, so some of the simplest wide char functions probably work correctly on old devices (e.g. wcscpy, wcsncpy, wcscat, wcslen, wcschr). libandroid_support doesn't have wcscpy, but it does have wcsncpy and the other functions I listed that AFAIK aren't necessary.

[DanAlbert]

Should we remove other broken wide char functions from headers and libc.map.txt? e.g. wcstol, wcstod, wprintf

Assuming you mean bump the introduction version to 21 where they start working, +1. We're not doing anyone any favors advertising support for broken APIs.

[enh-google]

Should we remove other broken wide char functions from headers and libc.map.txt? e.g. wcstol, wcstod, wprintf

doesn't ndk/sources/android/support/src/wcstox.cpp fix all the wcsto* functions?

wprintf() seems legitimately broken, but personally i'm happy ignoring anything we haven't explicitly had a bug report about... there's definitely a risk of removing something broken and breaking apps because they were compiling and/or linking against it, but not actually using it.

if mbstowcs() and wcstombs() are the only functions we've had bug reports for in 5+ years, that's probably all we actually need to worry about. especially since we shouldn't be too far off API 21 as the lowest supported API. i assume anyone supporting API < 21 in 2021 has an old app that "works", not anyone writing new code.

[DanAlbert]

There's still a little bit more work to do here. I had to flag a few tests as broken because although theses are backported in libandroid_support, the headers don't reflect that: https://android-review.googlesource.com/c/platform/ndk/+/1915003

I'm not sure what the best fix to this is. We can either remove the guard entirely (leading to a subpar diagnostic for C builds that don't use libandroid_support), or we could probably redeclare the APIs in the libandroid_support headers and lean on #pragma GCC system_header to suppress any redecl warnings (leading to a subpar diagnostic for C++ builds that don't use libandroid_support). There are probably other options too but those two are the ones that come to mind quickly.

[enh-google]

We can either remove the guard entirely (leading to a subpar diagnostic for C builds that don't use libandroid_support)

devil's advocate man asks: "now that libandroid_support isn't terrible, should we revisit that and just always have libandroid_support, and not make it a libc++ thing?"

[DanAlbert]

As fixed as this is going to get in r24 (works correctly now, UX not perfect for the expected-failing cases). Moving the cleanup work to r25.