Break when app and extapi module have a match

[Qcloud1223]

As name from app and extapi modules can only have exactly one match, we can break after find it.

The benefit is not limited to performance.

Take a look at the loop of the second break: fun is casted out of appfunc from funDefs, which is the function list of app module.
Then, fun->eraseFromParent() remove that function from the function list. This is ultimately a list->remove() operation, see: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Function.cpp#L389
This operation will release the object of fun, and also appfunc, so when we still try to compare appfunc with owfunc, the behavior is undefined.

I received a SIGSEGV at line 960 (original version) after thousands of calls to appfunc->getName() after the first match. Both the bc file and extapi are quite large so I prefer not pasting it here. Also since this issue is malloc-related, it's not easy to provide a stable yet minimal reproducible example, but I think describing it already makes much sense.

[yuleisui]

Would this patch fixing anything or introducing another bug potentially? Thanks

[codecov]

Codecov Report

Merging #1281 (db5ca34) into master (bf3ed56) will increase coverage by 0.02%.
Report is 68 commits behind head on master.
The diff coverage is 25.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1281      +/-   ##
==========================================
+ Coverage   64.44%   64.46%   +0.02%     
==========================================
  Files         223      223              
  Lines       23713    23813     +100     
==========================================
+ Hits        15281    15351      +70     
- Misses       8432     8462      +30     

Files	Coverage Δ
svf-llvm/lib/LLVMModule.cpp	82.77% <ø> (+0.32%)	⬆️
svf-llvm/tools/Example/svf-ex.cpp	36.66% <100.00%> (ø)
svf/lib/MemoryModel/AccessPath.cpp	3.12% <0.00%> (-0.54%)	⬇️

... and 23 files with indirect coverage changes

[Qcloud1223]

Would this patch fixing anything or introducing another bug potentially? Thanks

It will fix SIGSEGV on appfunc->getName() under rare cases, where overwriteExtFuncs is large enough, so that it will trigger many memory allocation/deallocation and eventually overwrite the memory used by original appfunc. Nobody has reported it though.

It will not introduce new bugs, since names in app module and extapi module should have exactly one match if they could.

[yuleisui]

Would this patch fixing anything or introducing another bug potentially? Thanks

It will fix SIGSEGV on appfunc->getName() under rare cases, where overwriteExtFuncs is large enough, so that it will trigger many memory allocation/deallocation and eventually overwrite the memory used by original appfunc. Nobody has reported it though.

It will not introduce new bugs, since names in app module and extapi module should have exactly one match if they could.

I don’t understand why many calls to ‘appfunc->getName’ will crash the program. Could you explain?

[yuleisui]

Make sense now if the overwrite function which has been freed and later used to cause this crash

[Qcloud1223]

I don’t understand why many calls to ‘appfunc->getName’ will crash the program. Could you explain?

Well I can't understand it at first...

The reason is, when the program enters the branch, it will eventually run fun->eraseFromParent(). fun is just an alias of appfunc due to Function* fun = const_cast<Function*>(appfunc);, so fun->eraseFromParent() will release appfunc.

However, when that object is released, it will not be zeroed out (for performance), but instead returns to the available pool of malloc. Since the line if (appfunc->getName().str().compare(owfunc->getName().str()) == 0) will create temporary objects, eventually the memory previously held by appfunc is reused and will fail appfunc->getName since appfunc is still pointing to the original memory address.