iOS Quiet Library for RN Bridging Allocates Memory on Heap without Freeing

[bitmold]

💧 this seems leaky. ios stuff seemingly hasn't been broken with this code because it's been in place for years, but it could be a very sneaky problem down the line, perhaps in some future version of iOS that apple will eventually force iOS devs to target this could lead to a possibly very sneaky crash. but hypothetical security issues or potential crashes aside, it's just not good to have leaks right?

Since's Android has a nearly identical C++ library for linking Quiet android to react native/nodejs land some of the changes I had made to freeing memory after calloc in this commit may also be useful for doing this work on iOS

A few leaky type things that stood out to me, though this is most likely nonexhaustive:

on iOS is that every time sendMessageToNode is called AppDelegate.m the heap allocated data that's stored in the local char pointer variable messageCopy in the subsequently invoked rn_bridge_notify is never free()d. I haven't modified any of the iOS RN Bridge library yet, but it seems at a glance 99.9% likely that you can and should put a call to free(messageCopy) after the call to channel->queueMessage(messageCopy)` at the end of the method to stop this leak.

void rn_bridge_notify(const char* channelName, const char *message) {
    int messageLength=strlen(message);
    char* messageCopy = (char*)calloc(sizeof(char),messageLength + 1);
    strncpy(messageCopy, message, messageLength);

    Channel* channel = GetOrCreateChannel(std::string(channelName));
    channel->queueMessage(messageCopy);
}

This leak is notable because rn_bridge_notify is potentially called many times during Quiet's execution, and so with every invocation more calloc()d memory leaks.

This one seems like less of a problem, because rn_register_node_data_dir_path is only invoked once in NodeRunner.mm

In the C++ though, if this method is invoked a second time datadir_path gets overwritten and the previously calloc()d memory for it leaks since it was never freed.

char* datadir_path = NULL;
/*
 * Called by the react-native plug-in to register the datadir,
 * representing a writable path. Expected to be called once,
 * while the plug-in initializes.
 */
void rn_register_node_data_dir_path(const char* path) {
    size_t pathLength = strlen(path);
    datadir_path = (char*)calloc(sizeof(char), pathLength + 1);
    strncpy(datadir_path, path, pathLength);
}

Later on datadir_path is never free()d after it's use in Method_GetDataDir

/**
 * Get the registered datadir
 */
napi_value Method_GetDataDir(napi_env env, napi_callback_info info) {
  NAPI_ASSERT(env, datadir_path!=NULL, "Data directory not set from native side.");
  napi_value return_datadir;
  size_t str_len = strlen(datadir_path);
  NAPI_CALL(env, napi_create_string_utf8(env, datadir_path, str_len, &return_datadir));
  // never freed
  return return_datadir;
}

[holmesworcester]

Would this bug affect other users of NodeJS Mobile, or is this specific to some way we're using or misusing it?

[bitmold]

No, this wasn't about nodejs mobile. It was a minor problem in the iOS app's C++ homerolled react native bridge library (and also until recently, the equivalent android C++ quiet project had similar issues)

Although, in a certain sense, other users of nodejsmobile most likely also have their own versions of this problem because it's unfortunately very easy for even the most battle hardened C/C++ vets to accidentally leak memory 🤷‍♀️

But this stuff in Quiet's small C++ codebase wasn't so much a bug - afaik it didn't seem to cause crashes/gobble up resources/cause undefined behavior/etc...

In C/C++ you can manually allocate memory off the heap via methods like malloc(), calloc(). Unlike stack based memory which exists only in the scope a function and is automatically reclaimed by the OS when that scope ends, memory from the heap lingers eternally and you're 100% responsible for manually releasing it back to the OS via free() method. Otherwise,, that RAM is permanently claimed by Quiet (even if the program has moved on and stopped caring about it) and the OS won't get it back until the program terminates - ---- this often leads to hard to pin down, devilish nightmare genres of problems, but in Quiet's case the leaked memory was small and seemingly inconsequential.

Still though, this type of thing is bad practice, so in the interest of avoiding any possible problems down the line/to prevent any confusion someone in the future might have when reading through the C++ project, I free()d the offending stuff when working with these files during the nodejsmobile upgrade. so closing this.

[bitmold]

*and rn_bridge_notify seemingly does free() its memory, I didn't realize it when I opened the ticket, but it seems to in flushQueue() https://github.com/TryQuiet/quiet/blob/develop/packages/mobile/ios/NodeJsMobile/rn-bridge.cpp#L150