facebook/redex

[Question] why is 'kInsertDeobfuscatedNameLinks' set to false

fengruisd opened this issue · 9 comments

hi, I have a question,

why do you set the default value of kInsertDeobfuscatedNameLinks to be false?

and then DexType::get_type("deobfuscated name") will return null, that really confuse me.

I guess we don't need to maintain two name mappings for a type when running Redex. Redex produces the obfuscation name mapping at the end. Optimizations depending on deobfuscated symbols can use get_deobfuscated_name?

I see. but when you set kInsertDeobfuscatedNameLinks to be false, the deobfuscated name won't be set into g_redex.

and for for deobfuscated name, DexType::get_type will always return nullptr and bind("key", {}, std::vector<DexType*> some_config) will never bind the right value.

DexType::get_type and bind(...) are used extensively in the code, so i worried that the flag may cause some problems

I see. but when you set kInsertDeobfuscatedNameLinks to be false, the deobfuscated name won't be set into g_redex.

Yes, that's what the option is for, not to establish the name link in the global context when set to false. But the m_deobfuscated_name member still stores the obfuscated symbol.

DexType::get_type and bind(...) are used extensively in the code, so i worried that the flag may cause some problems

The convention is to run the obfuscate/rename passes later in the pipeline. So the original symbol is linked till reaching the later stage of the optimization passes. Config binding happens in pass evaluation phase. That's before running the actual passes, therefore before rename/obfuscation takes place. Hopefully that makes sense.

The convention is to run the obfuscate/rename passes later in the pipeline. So the original symbol is linked till reaching the later stage of the optimization passes. Config binding happens in pass evaluation phase. That's before running the actual passes, therefore before rename/obfuscation takes place. Hopefully that makes sense.

I think I've got your point. your application does not use Proguard or R8, so classes are not obfuscated before being processed by redex. But our application is integrated with Proguard, which is why DexType::get_type returns nullptr in our pipeline.

thanks

Ah IC. Then perhaps it makes more sense for you to keep the link. We added the option to save memory, I think.

@agampe Seems like it makes more sense to set kInsertDeobfuscatedNameLinks to true for OSS users?

Even when it's set to true things are at least slightly broken. Redex did not correctly manage lifetimes, which led to leaks all over the place, and in some cases to stale pointers. (Yes, this was found while doing memory work, but nonetheless things were broken.)

If you have to use Proguard (which we don't advise, and if there are missing optimizations, please file issues), then please flip the switch locally. I don't think adding the support via conditional compilation and compiler defines is worth the complexity.

Even when it's set to true things are at least slightly broken. Redex did not correctly manage lifetimes, which led to leaks all over the place, and in some cases to stale pointers. (Yes, this was found while doing memory work, but nonetheless things were broken.)

redex just insert some new elements into s_xxx_map when it's set to true, it does consume more memory, but why does this behavior cause new memory leak issues?

Because these were not correctly removed in all cases when the "canonical" element went away. It's not a "leak leak," there are still references, but it's stale links for things that should be gone.