[BUG] Bug in stpncpy implementation: Wrong pointer returned

[Menkalian]

Description

Bug Description

When using NDK r29 stpncpy does not return the correct pointer in some cases.
The issue is present when src has a size known at compile time. In that case __stpncpy_chk2 is used which always returns the dest pointer and not a pointer to the end of the string. This is not conform to stpncpy as it is defined in the C standard.

Note: The behavior can only be observed when BIONIC FORTIFY is used. This is the default behavior when using the NDK as part of building an application.

Standalone C example

#include <string.h>
#include <stdio.h>

const char prefix[] = "prefix_";
const char *text = "FooBar";

int main(int argc, char **argv) {
    char buffer[20];
    size_t buffer_len = sizeof(buffer) - 1;
    char *cp;

    cp = stpncpy(buffer, prefix, buffer_len);
    buffer_len -= sizeof(prefix) - 1;
    cp = stpncpy(cp, text, buffer_len);

    printf("Output: %s\n", buffer);
    return 0;
}

This has to be compiled with ${ANDROID_CC} -D_FORTIFY_SOURCE=2 testcase_stpncpy.c -o testcase_stpncpy.
The program will output Output: FooBar, but the correct output should be Output: prefix_FooBar.

Compiling the program with either gcc or clang on my host machine (x86_64, Linux in WSL) does not exhibit this issue, thus it is an issue with the NDK Toolchains.

Example Android Application

StpncpyExample.zip contains a small sample app which uses the above codesnippet to display some text. It exhibits the same behavior.

I am using a supported NDK

• I have checked and the NDK I'm using is currently supported

Affected versions

r29

[enh-google]

yeah, definitely a bug in the __stpncpy_chk2() implementation. sadly that's part of the OS rather than being inline in the ndk headers. i'm arguing internally that we should probably inline a fixed version so everyone benefits immediately.

(interestingly this is not a regression: it's been wrong since it was introduced, over a decade ago.)

in the meantime you can either -U_FORTIFY_SOURCE to disable fortify entirely, or edit your headers to remove the call to __stpncpy_chk2() so you get most of what fortify has to offer but avoid this bug. (this is my backup suggestion for a long-term fix, if there's a reason not to go with "just inline the correct implementation into the header".)

[enh-google]

i've merged a fix for this internally: from api 28, __stpncpy_chk2() does the right thing, and if you're compiling with _FORTIFY_SOURCE against anything earlier, you'll only get __stpncpy_chk() instead (since there's no obvious cheap way to compute the correct return value).

this will probably be cherrypicked into the ndk r30 sysroot ... but over to the ndk folks for that.

in the meantime, if you want to patch this locally, replace the stpncpy() inline in your libc/include/bits/fortify/string.h with this:

__BIONIC_FORTIFY_INLINE
char* _Nonnull stpncpy(char* _Nonnull const dst __pass_object_size, const char* _Nonnull const src __pass_object_size, size_t n)
        __fortify_overload
        __diagnose_as_builtin(__builtin_stpncpy, 1, 2, 3) {
    size_t bos_dst = __bos(dst);
    size_t bos_src = __bos(src);

    /*
     * Ignore dst size checks; they're handled in stpncpy_chk.
     * __stpncpy_chk2() is broken pre-38, returning dst.
     */
    if (__ANDROID_API__ < 38 || bos_src == __BIONIC_FORTIFY_UNKNOWN_SIZE) {
        return __builtin___stpncpy_chk(dst, src, n, bos_dst);
    }

    return __stpncpy_chk2(dst, src, n, bos_dst, bos_src);
}