appending values each time when calling JNI method

Issue

i am using a JNI to fetch signature of apk and i am getting it very well. when i calling this method from java first time i am getting the exact value. calling it again i am getting appended values with exact value (eg 1234456123456). PFB the code which i am using

char* getSignatureMd5(JNIEnv* env, jobject obj)
{
    char* sign = loadSignature(env, obj);
    MD5_CTX context = { 0 };
    MD5Init(&context);
    MD5Update(&context, (unsigned char*)sign, strlen(sign));
    unsigned char dest[16] = { 0 };
    MD5Final(dest, &context);
    int i;
    static char destination[32]={0};
    for (i = 0; i < 16; i++) {
            sprintf(destination, "%s%02x", destination, dest[i]);
    }
    return destination;
}

getToken JNI method

JNIEXPORT jstring JNICALL Java_com_sign_signaturecapturesbi_MyAdapter_getToken(JNIEnv *env, jobject obj)
                                                                     {
    char* signValue = getSignatureMd5(env, obj);
    __android_log_print(ANDROID_LOG_VERBOSE, "MyApp", "signValue %s", signValue);
    return (*env)->NewStringUTF(env, signValue);
    }

Solution

These lines cause undefined behavior:

for (i = 0; i < 16; i++) {
        sprintf(destination, "%s%02x", destination, dest[i]);
}

man 3 printf:

C99 and POSIX.1-2001 specify that the results are undefined if a call to sprintf(), snprintf(), vsprintf(), or vsnprintf() would cause copying to take place between objects that overlap (e.g., if the target string array and one of the supplied input arguments refer to the same buffer).

Moreover destination is static and because of this it keeps its content between calls. Together these points give you such a weird behavior.

Since dest size is well known, you can simply unroll loop, also don't forget to add one extra cell to destination for terminating \0. And, if possible, you should use snprintf() instead:

static char destination[33];

snprintf(destination, sizeof destination,
    "%02x%02x%02x%02x%02x%02x%02x%02x"
    "%02x%02x%02x%02x%02x%02x%02x%02x",
    dest[0], dest[1], dest[2], dest[3],
    dest[4], dest[5], dest[6], dest[7],
    dest[8], dest[9], dest[10], dest[11],
    dest[12], dest[13], dest[14], dest[15]);

In this case you can leave destination as static one, since your code doesn't relay on its content anymore. But note that getSignatureMd5() returns pointer to the same buffer each time you call it, as result subsequent calls erase result obtained by previous calls.

Answered By - Sergio