Fix bad memory access issue on handleMemoryPressure function

[bwisn]

Summary:

This PR solves the problem with crashing of the RN app, while running garbage collector on iOS. App was crashing when received low memory warning from OS. This was caused by trying to read from uninitialized variable const char* levelName. The variable wasn't initialized, so it might contain a random address. It should be initialized using malloc, or initialized as constant length one. Second issue was that in the later part, string was just assigned to this variable. This caused a segmentation fault on iOS, because the internal C++ stream operator<< was trying to run strlen() on this uninitialized variable.

I solved that by changing the type of variable from const char* to std::string and adding an adequate include.

The bug was introduced by commit 48001c5 on Mar 6, 2020 and version v0.63.0

It's present in the code up to today. I created this bugfix branch from 0.75-stable branch, as it is the oldest still supported version.

Changelog:

[GENERAL] [FIXED] - Fixed logger issue which caused app crash while running GC, caused by wrong variable type

Test Plan:

Start many (+- 20) apps, which uses a lot of memory, put them in background, then start your app (our app is proprietary, so no MWE available), which uses a lot of memory. In our case, the app was using +500MB of memory (memory intensive task).

After receiving the notification from the system, app will crash on strlen(). (below)

Then apply the fix, and the issue will gone. You'll see in the debug console correct logs, and app will no longer crash. (below)

This was reproduced on iPhone 11 with iOS 18, iPhone 12 with iOS 18, iPhone 14 Pro with iOS 18

[facebook-github-bot]

Hi @bwisn!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[javache]

const char * should be fine here. Can you maybe just make the default value "UNKNOWN"?

[bwisn]

@javache In my opinion, std::string will be much safer in terms of memory. If you want to use const char*, we should use malloc to allocate a buffer which will be big enough, zero it, and then use strncpy (with n less than buffer size) to make sure that we don't have buffer overflow.

If you just assign "UNKNOWN" to the variable and try to assign a longer string (longer than 7 characters + \0), the program will try to write it under the address which is stored under variable levelName, but there is only 8 bytes reserved (due to "UNKNOWN". This will lead to undefined behaviours.

[javache]

@bwisn That's not how const char * works in C++. Any static string defined here will have a permanent address in the binary, and we can safely pass around pointers to those strings.

The last thing we want to do in this memory callback is allocate more memory on the heap.

[bwisn]

@javache my bad, you're right - I thought that the pointer is const, not the variable stored under it.

But unfortunately, I've tried to set the default value for the levelName ... and it doesn't work. It crashes on the << operator again. Below you can see a screenshot from crash and backtrace from lldb. My change with std::string work flawlessly - const char* with default doesn't.

[bwisn]

IMO compiler probably does something strange with that const char* while optimizing.

[javache]

What's the pressureLevel in that case? It seems to be trying to run strlen on an empty string, so something's overriding levelName. I'm skeptical that the compiler would be incorrectly optimizing this type of basic pattern.

[bwisn]

@javache I don't have the phone at the moment with me, but as far as I remember, it was the CRITICAL one. I can't see anything in that function what could override the levelName. And it should be set to default (in the case of unknown pressureLevel) to UNKNOWN (in the switch), so how that would be possible?

[bwisn]

@javache as far as i can see, the pressureLevel will always be 15 because:

in file packages/react-native/React/CxxBridge/RCTCxxBridge.mm handleMemoryPressure is called, and the argument is the pressureLevel which is read from static variable

as you can see here (file packages/react-native/React/Base/RCTConstants.m)

The default value is 15 which means TRIM_MEMORY_RUNNING_CRITICAL and the RCTSetMemoryPressureUnloadLevel is never used anywhere in the code as you can see below.

[bwisn]

@javache also, the variable static int RCTMemoryPressureUnloadLevel = 15; shouldn't be static volatile int RCTMemoryPressureUnloadLevel = 15; to make sure that compiler won't optimize this if the variable is never changing? At this moment it isn't changing for iOS, but who knows the future ;)

[bwisn]

@javache I've found that in RCTCxxBridge.mm level always equals 15, but something happens when it's passed to C++ (Instance.cpp), sometimes it's changing to 39, 56, etc. I'm still trying to find what is going on here.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	20,743,812	-611,044
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	23,945,545	-604,446
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: fcd526d
Branch: main

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!