Fix malloc&memcpy issue in exec_am_broadcast #203

Grimler91 · 2025-03-26T21:13:58Z

If termux-am-socket is not used, then termux-api falls back to the slower exec_am_broadcast method.

If we strip exec_am_broadcast down to something shorter it essentially runs:

minimal example of original code

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

void exec_am_broadcast(int argc, char **argv)
{
    int const extra_args = 2; // Including ending NULL.
    char** child_argv = malloc((sizeof(char*)) * (argc + extra_args));

    child_argv[0] = "am";
    child_argv[1] = argv[1];

    // Copy the remaining arguments -2 for first binary and second api name:
    memcpy(child_argv + extra_args, argv + 2, (argc-1) * sizeof(char*));

    // End with NULL:
    child_argv[argc + extra_args - 1] = NULL;

    // free added here, not strictly needed in original since function is _Noreturn, but it is needed to make valgrind happy
    free(child_argv);
}

int main(int argc, char **argv)
{
    exec_am_broadcast(argc, argv);
    return 0;
}

If we compile this and run this with a memory checker like valgrind:

gcc -o test orig-test.c
valgrind --leak-check=full --show-leak-kinds=all -s --track-origins=yes ./test foo bar baz

then there are no issues (a NULL termination issue was fixed earlier in
commit 08bcd7331982 ("exec_am_broadcast: fix NULL assignment of last element of child_argv") though).

Now let's modify the example to print all the child_argv to see what we have actually set the array to:

minimal example with print based on original code

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

void exec_am_broadcast(int argc, char **argv)
{
    int const extra_args = 2; // Including ending NULL.
    char** child_argv = malloc((sizeof(char*)) * (argc + extra_args));

    child_argv[0] = "am";
    child_argv[1] = argv[1];

    // Copy the remaining arguments -2 for first binary and second api name:
    memcpy(child_argv + extra_args, argv + 2, (argc-1) * sizeof(char*));

    // End with NULL:
    child_argv[argc + extra_args - 1] = NULL;

    for (int i=0; i<argc+extra_args; i++) {
	printf("child_argv[%d]: %s\n", i, child_argv[i]);
    }

    // free added here, not strictly needed in original since function is _Noreturn, but it is needed to make valgrind happy
    free(child_argv);
}

int main(int argc, char **argv)
{
    exec_am_broadcast(argc, argv);
    return 0;
}

If we compile and run under valgrind it still works fine, but we can see that it contains two NULLs at the end:

child_argv[0]: am
child_argv[1]: foo
child_argv[2]: bar
child_argv[3]: baz
child_argv[4]: (null)
child_argv[5]: (null)

which is unnecessary, clearly there is a mistake here, but two NULLs at the end does not really cause any issues at this point.

Now let's add a function that calls exec_am_broadcast with some args "manually" (I am doing this in a termux-usb improvement that I am working on):

example with call to exec_am_broadcast from another function

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

void exec_am_broadcast(int argc, char **argv)
{
    int const extra_args = 2; // Including ending NULL.
    char** child_argv = malloc((sizeof(char*)) * (argc + extra_args));

    child_argv[0] = "am";
    child_argv[1] = argv[1];

    // Copy the remaining arguments -2 for first binary and second api name:
    memcpy(child_argv + extra_args, argv + 2, (argc-1) * sizeof(char*));

    // End with NULL:
    child_argv[argc + extra_args - 1] = NULL;

    for (int i=0; i<argc+extra_args; i++) {
        printf("child_argv[%d]: %s\n", i, child_argv[i]);
    }

    // free added here, not strictly needed in original since function is _Noreturn, but it is needed to make valgrind happy
    free(child_argv);
}

void custom_exec_am_broadcast()
{
    int argc = 3;
    char *argv[argc];

    argv[0] = "foo";
    argv[1] = "bar";
    argv[2] = "baz";

    exec_am_broadcast(argc, argv);
}

int main(int argc, char **argv)
{
    custom_exec_am_broadcast();
    return 0;
}

If we compile this an run it under valgrind as before we get:

$ valgrind --leak-check=full --show-leak-kinds=all -s --track-origins=yes ./test 
==20918== Memcheck, a memory error detector
==20918== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==20918== Using Valgrind-3.24.0 and LibVEX; rerun with -h for copyright info
==20918== Command: ./test
==20918== 
child_argv[0]: am
child_argv[1]: bar
child_argv[2]: baz
==20918== Conditional jump or move depends on uninitialised value(s)
==20918==    at 0x48FE8E4: __printf_buffer (vfprintf-process-arg.c:408)
==20918==    by 0x49011E3: __vfprintf_internal (vfprintf-internal.c:1544)
==20918==    by 0x48F4EB2: printf (printf.c:33)
==20918==    by 0x109259: exec_am_broadcast (in /home/grimler/termux-api-package/test)
==20918==    by 0x109326: custom_exec_am_broadcast (in /home/grimler/termux-api-package/test)
==20918==    by 0x10935D: main (in /home/grimler/termux-api-package/test)
==20918==  Uninitialised value was created by a stack allocation
==20918==    at 0x10927A: custom_exec_am_broadcast (in /home/grimler/termux-api-package/test)
==20918== 
child_argv[3]: (null)
child_argv[4]: (null)
==20918== 
==20918== HEAP SUMMARY:
==20918==     in use at exit: 0 bytes in 0 blocks
==20918==   total heap usage: 2 allocs, 2 frees, 1,064 bytes allocated
==20918== 
==20918== All heap blocks were freed -- no leaks are possible
==20918== 
==20918== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
==20918== 
==20918== 1 errors in context 1 of 1:
==20918== Conditional jump or move depends on uninitialised value(s)
==20918==    at 0x48FE8E4: __printf_buffer (vfprintf-process-arg.c:408)
==20918==    by 0x49011E3: __vfprintf_internal (vfprintf-internal.c:1544)
==20918==    by 0x48F4EB2: printf (printf.c:33)
==20918==    by 0x109259: exec_am_broadcast (in /home/grimler/termux-api-package/test)
==20918==    by 0x109326: custom_exec_am_broadcast (in /home/grimler/termux-api-package/test)
==20918==    by 0x10935D: main (in /home/grimler/termux-api-package/test)
==20918==  Uninitialised value was created by a stack allocation
==20918==    at 0x10927A: custom_exec_am_broadcast (in /home/grimler/termux-api-package/test)
==20918== 
==20918== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Oops, child_argv[3] has uninitialised memory. I have no idea why the previous example (where we ran the same but with argv from command line) did not give the same error, maybe someone smarter can explain this?

We can fix our example by memcpy'ing to the child_argv element AFTER the one we set to argv[1], and only copying argc-2 elements (since we start at argv+2):

fixed example

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

void exec_am_broadcast(int argc, char **argv)
{
    int const extra_args = 1; // Including ending NULL.
    char** child_argv = malloc((sizeof(char*)) * (argc + extra_args));

    child_argv[0] = "am";
    child_argv[1] = argv[1];

    // Copy the remaining arguments -2 for first binary and second api name:
    memcpy(child_argv + extra_args + 1, argv + 2, (argc-2) * sizeof(char*));

    // End with NULL:
    child_argv[argc + extra_args - 1] = NULL;

    for (int i=0; i<argc+extra_args; i++) {
        printf("child_argv[%d]: %s\n", i, child_argv[i]);
    }

    // free added here, not strictly needed in original since function is _Noreturn, but it is needed to make valgrind happy
    free(child_argv);
}

void custom_exec_am_broadcast()
{
    int argc = 3;
    char *argv[argc];

    argv[0] = "foo";
    argv[1] = "bar";
    argv[2] = "baz";

    exec_am_broadcast(argc, argv);
}

int main(int argc, char **argv)
{
    custom_exec_am_broadcast();
    return 0;
}

We then end up with:

child_argv[0]: am
child_argv[1]: bar
child_argv[2]: baz
child_argv[3]: (null)

and no issues when running under valgrind. Hooray!

In the PR I have done essentially the fix described above, and some extra cleanup. See the individual commit messages for the rationale for each small change.

Grimler91 · 2025-03-26T21:29:33Z

termux-api.c

 #include <stdlib.h>
 #include <string.h>
-#include <sys/endian.h>
+#ifdef __TERMUX


Typo here, but apparently it works for termux as well if arpa/inet.h is included instead, so I'll update and include that header always

agnostic-apollo · 2025-03-27T15:22:03Z

termux-api.c


-    int const extra_args = 15; // Including ending NULL.
+    int const extra_args = 14; // Including ending NULL.
    char** child_argv = malloc((sizeof(char*)) * (argc + extra_args));


There are confusions and wrong usage of extra_args previously and in your changes too.

There are 13 elements that are assigned to child_argv, till api_method extra key. For this you need an array of size 14, not 13, considering 0 indexing. The Including ending NULL comment here is wrong, its not being included here, its "accidentally" being included via argc in argc + extra_args in malloc() as you are not copying argv[0], leaving one extra space for NULL, resulting in correct execution despite code logic being wrong.

Secondly, trying to add a space for NULL in extra_args is wrong too, as its then used as memcpy() for indexing. Again the memcpy() is "accidentally" working correctly. The copying must start at index 14, after the 13 pre args index, and it is working because the space for NULL added compensated for the wrong size of extra_args.

Additionally note that malloc() result should not be directly cast to char ** and stored in void * first, otherwise can trigger SIGABRT (null pointer dereference) on Android if NULL is returned, at least for the native JNI code in app.

The Including ending NULL comment here is wrong, its not being included here

Indeed, I'll update the comment.

its not being included here, its "accidentally" being included via argc in argc + extra_args in malloc() as you are not copying argv[0], leaving one extra space for NULL

Yes, that was my thinking, since we don't cpy argv[0] that space can be used for NULL instead

Additionally note that malloc() result should not be directly cast to char ** and stored in void * first, otherwise can trigger SIGABRT (null pointer dereference) on Android if NULL is returned

Ok, I'll change that!

agnostic-apollo · 2025-03-27T15:29:46Z

termux-api.c

+    memcpy(child_argv + extra_args, argv + 1, (argc-1) * sizeof(char*));

    // End with NULL:
    child_argv[argc + extra_args - 1] = NULL;


Now here note that you argc is wrong, we didn't copy argv[0], so it shouldn't have been counted in size, and that's why there was as exception before 08bcd73, not because of 0 indexing. If argc size used was correct and since we had added an extra space for NULL, then -1 wouldn't have been required.

agnostic-apollo · 2025-03-27T15:35:03Z

Use following, apart from free() and header include. This should make things more clear.

    const int child_pre_argc = 14;
    const int child_post_argc = argc - 1; // Except `argv[0]`.
    const int child_argc = child_pre_argc + child_post_argc;

    size_t child_argv_size = (sizeof(char*)) * (child_argc + 1); // Including trailing `NULL`.
    // Do not directly cast, otherwise can trigger null pointer dereference if `NULL` is returned.
    void* result = malloc(child_argv_size);
    if (result == NULL) {
        perror("malloc failed for am child args");
        exit(1);
    }

    char **child_argv = (char **) result;

    child_argv[0] = "am";
    child_argv[1] = "broadcast";
    child_argv[2] = "--user";
    child_argv[3] = "0";
    child_argv[4] = "-n";
    child_argv[5] = "com.termux.api/.TermuxApiReceiver";
    child_argv[6] = "--es";
    // Input/output are reversed for the java process (our output is its input):
    child_argv[7] = "socket_input";
    child_argv[8] = output_address_string;
    child_argv[9] = "--es";
    child_argv[10] = "socket_output";
    child_argv[11] = input_address_string;
    child_argv[12] = "--es";
    child_argv[13] = "api_method";

    // Copy the remaining arguments except `argv[0]`, `argv[1]` should be `api_method` extra value:
    memcpy(child_argv + child_pre_argc, argv + 1, child_post_argc * sizeof(char*));

    // End with NULL:
    child_argv[child_argc] = NULL;

    for (int i=0; i<=child_argc; i++) {
        fprintf(stderr, "child_argv[%d]: %s\n", i, child_argv[i]);
    }

$ termux-sms-list
child_argv[0]: am
child_argv[1]: broadcast
child_argv[2]: --user
child_argv[3]: 0
child_argv[4]: -n
child_argv[5]: com.termux.api/.TermuxApiReceiver
child_argv[6]: --es
child_argv[7]: socket_input
child_argv[8]: 1fd6a73a6f951c15-10f2-4e09-b495-44b2c360e58d9347a693b220
child_argv[9]: --es
child_argv[10]: socket_output
child_argv[11]: d4b68014c2039b1c-10f2-4434-84fc-c183a2e6a57a9db1becf8747
child_argv[12]: --es
child_argv[13]: api_method
child_argv[14]: SmsInbox
child_argv[15]: --ei
child_argv[16]: limit
child_argv[17]: 10
child_argv[18]: --ei
child_argv[19]: offset
child_argv[20]: 0
child_argv[21]: --ei
child_argv[22]: type
child_argv[23]: 0
child_argv[24]: (null)
[]

The header is included to get defintion for htons. In termux the definition is in sys/endian.h, but on other systems it is in arpa/inet.h. arpa/inet.h includes sys/endian.h though, so including that header works in termux as well. Change to arpa/inet.h so that termux-api-package can be easily compiled both in termux and for other systems.

We exit right after so this is not exactly strictly necessary, but that might change at some point and is good practise in any case.

malloc might fail to allocate memory for child_argv. Ensure it was successfully allocated before trying to set its elements.

agnostic-apollo · 2025-03-28T20:28:53Z

Thanks. Looks good to me, merge away, might wanna add link to your and my comment in the bytes commit message.

And make malloc&memcpy handling clearer. Before we potentially had an element with uninitialised memory, see discussion in termux#203. Fixes: 1ec1334 ("Initial push")

Grimler91 commented Mar 26, 2025

View reviewed changes

agnostic-apollo reviewed Mar 27, 2025

View reviewed changes

Grimler91 force-pushed the exec-am-broadcast-malloc-fix branch from af865a2 to 1349459 Compare March 28, 2025 16:58

Grimler91 added 3 commits March 28, 2025 17:58

termux-api: free malloc'ed child_argv array on execv error

530f292

We exit right after so this is not exactly strictly necessary, but that might change at some point and is good practise in any case.

termux-api: handle malloc errors

512a252

malloc might fail to allocate memory for child_argv. Ensure it was successfully allocated before trying to set its elements.

Grimler91 force-pushed the exec-am-broadcast-malloc-fix branch from 1349459 to cb9cb06 Compare March 28, 2025 17:02

Fixed: only malloc as many bytes as are actully needed

0edd4d4

And make malloc&memcpy handling clearer. Before we potentially had an element with uninitialised memory, see discussion in termux#203. Fixes: 1ec1334 ("Initial push")

Grimler91 force-pushed the exec-am-broadcast-malloc-fix branch from cb9cb06 to 0edd4d4 Compare March 29, 2025 21:02

Grimler91 merged commit 0edd4d4 into termux:master Mar 29, 2025

Grimler91 deleted the exec-am-broadcast-malloc-fix branch March 29, 2025 21:03

omidbaratiy mentioned this pull request Aug 10, 2025

Use following, apart from free() and header include. This should make things more clear. omidbaratiy/Omiddha#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix malloc&memcpy issue in exec_am_broadcast #203

Fix malloc&memcpy issue in exec_am_broadcast #203

Uh oh!

Grimler91 commented Mar 26, 2025

Uh oh!

Grimler91 Mar 26, 2025

Uh oh!

agnostic-apollo Mar 27, 2025

Uh oh!

agnostic-apollo Mar 27, 2025

Uh oh!

Grimler91 Mar 28, 2025

Uh oh!

agnostic-apollo Mar 27, 2025

Uh oh!

agnostic-apollo commented Mar 27, 2025 •

edited

Loading

Uh oh!

agnostic-apollo commented Mar 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix malloc&memcpy issue in exec_am_broadcast #203

Fix malloc&memcpy issue in exec_am_broadcast #203

Uh oh!

Conversation

Grimler91 commented Mar 26, 2025

Uh oh!

Grimler91 Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

agnostic-apollo Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

agnostic-apollo Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

Grimler91 Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

agnostic-apollo Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

agnostic-apollo commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agnostic-apollo commented Mar 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

agnostic-apollo commented Mar 27, 2025 •

edited

Loading