Fix statically allocated objects #1688

adriaanjacobs · 2025-04-09T17:12:49Z

Hi

As discussed in #1680, this patch largely reverts #1616, which mistakenly seems to refer to statically-allocated objects as "stack-allocated objects". As far as we can tell, this lead to only 1 bug in SVF, in the SVFIRBuilder, where a regular isa<Alloca> was replaced with a function to check for a call to an extapi func returning "stack-allocated storage" (which actually returned an unknown static object). That's obviously wrong. Fixed in 4d7ece4e5b4.

However, we (surprisingly!) did not notice any build failures after updating this naming convention and deleting some utility functions. Does that mean that SVF does not check for statically-allocated objects in its handling of external calls? I'm confused here: where should we find & update t 8000 he code that treats all calls to such functions as aliasing?

I believe there's more work to be done to properly handle these STATIC_RET functions. @mortkever/@shuangxiangkan any ideas?

codecov · 2025-04-09T17:23:42Z

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.60%. Comparing base (c63be58) to head (db3fabe).

Files with missing lines	Patch %	Lines
svf-llvm/lib/LLVMModule.cpp	0.00%	2 Missing ⚠️
svf/lib/Util/ExtAPI.cpp	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
+ Coverage   63.58%   63.60%   +0.01%     
==========================================
  Files         244      244              
  Lines       25741    25730      -11     
  Branches     4510     4508       -2     
==========================================
- Hits        16368    16365       -3     
+ Misses       9373     9365       -8

Files with missing lines	Coverage Δ
svf-llvm/include/SVF-LLVM/LLVMModule.h	`97.72% <ø> (ø)`
svf-llvm/include/SVF-LLVM/LLVMUtil.h	`77.58% <ø> (+1.31%)`	⬆️
svf-llvm/lib/LLVMUtil.cpp	`75.47% <ø> (+1.67%)`	⬆️
svf-llvm/lib/SVFIRBuilder.cpp	`86.81% <100.00%> (ø)`
svf/include/Util/ExtAPI.h	`0.00% <ø> (ø)`
svf-llvm/lib/LLVMModule.cpp	`75.47% <0.00%> (ø)`
svf/lib/Util/ExtAPI.cpp	`41.57% <0.00%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

8000

yuleisui · 2025-04-22T11:49:33Z

@adriaanjacobs I finally had some time to review this pull request. ALLOC_STACK_RET (stack lifetime) and STATIC_RET (program lifetime) represent two different types of objects. I would suggest retaining both. Are you certain that all ALLOC_STACK_RET annotations should be replaced with STATIC_RET? We may need to keep both for future annotations and extend external APIs.

I don't think we have to change this line here, as isStackObj makes more sense to me if we keep these two types of annotations.

Please let me know your thoughts and update the patch accordingly.

Thanks.

adriaanjacobs · 2025-04-22T16:33:23Z

Thanks for taking a look.

I don't think I fully understand what the ALLOC_STACK_RET is supposed to mean. How can functions return stack-allocated data? Is that not always a stack-use-after-free? E.g.

void* func() {
    char data[SIZE];
    return &data[0];
}

I've confirmed that all changed ExtAPI functions were previously (before #1616) STATIC_OBJECT functions, so I'm not sure which function in ExtAPI is an example of a function SVF would like to handle as ALLOC_STACK_RET? Do you happen to know what the original intention behind #1616 was, i.e., for which ExtAPI function was it introduced?

yuleisui · 2025-04-23T01:10:45Z

Thanks for taking a look.

I don't think I fully understand what the ALLOC_STACK_RET is supposed to mean. How can functions return stack-allocated data? Is that not always a stack-use-after-free? E.g.
void* func() {
    char data[SIZE];
    return &data[0];
}
I've confirmed that all changed ExtAPI functions were previously (before #1616) STATIC_OBJECT functions, so I'm not sure which function in ExtAPI is an example of a function SVF would like to handle as ALLOC_STACK_RET? Do you happen to know what the original intention behind #1616 was, i.e., for which ExtAPI function was it introduced?

Some c standard library API like void* alloca(size_t size); can return a stack object which is similar to LLVM's alloca instruction. Hence we may need to keep ALLOC_STACK_RET, isStackObj, is_static_ret methods and so on.

I had another look at extapi.c, I agree that all of the current ALLOC_STACK_RET annotations should be STATIC_OBJECT. But we need to handle each API call and allocate for a new global/static object by changing a new condition here:

else if (SVFUtil::isa<GlobalValue>(llvmValue) || is_static_ret_obj(llvmValue))
{

}

Let me know your thoughts.

shuangxiangkan · 2025-04-23T01:17:06Z

Thanks for taking a look.

I don't think I fully understand what the ALLOC_STACK_RET is supposed to mean. How can functions return stack-allocated data? Is that not always a stack-use-after-free? E.g.
void* func() {
    char data[SIZE];
    return &data[0];
}
I've confirmed that all changed ExtAPI functions were previously (before #1616) STATIC_OBJECT functions, so I'm not sure which function in ExtAPI is an example of a function SVF would like to handle as ALLOC_STACK_RET? Do you happen to know what the original intention behind #1616 was, i.e., for which ExtAPI function was it introduced?

The original design didn't distinguish between external functions allocating static objects or stack objects—it essentially treated them all as stack objects. So, simply reverting #1616 isn't enough, which is also why you didn't encounter build failures in #1688, as static object allocation for external functions wasn't addressed.

yuleisui · 2025-04-24T12:36:07Z

Thanks for taking a look.
I don't think I fully understand what the ALLOC_STACK_RET is supposed to mean. How can functions return stack-allocated data? Is that not always a stack-use-after-free? E.g.
void* func() {
    char data[SIZE];
    return &data[0];
}
I've confirmed that all changed ExtAPI functions were previously (before #1616) STATIC_OBJECT functions, so I'm not sure which function in ExtAPI is an example of a function SVF would like to handle as ALLOC_STACK_RET? Do you happen to know what the original intention behind #1616 was, i.e., for which ExtAPI function was it introduced?
Some c standard library API like void* alloca(size_t size); can return a stack object which is similar to LLVM's alloca instruction. Hence we may need to keep ALLOC_STACK_RET, isStackObj, is_static_ret methods and so on.

I had another look at extapi.c, I agree that all of the current ALLOC_STACK_RET annotations should be STATIC_OBJECT. But we need to handle each API call and allocate for a new global/static object by changing a new condition here:
else if (SVFUtil::isa<GlobalValue>(llvmValue) || is_static_ret_obj(llvmValue))
{

}
Let me know your thoughts.

@adriaanjacobs could you check the above the update the pull request and let me know if any questions

mortkever · 2025-04-24T22:37:30Z

Thanks for taking a look.
I don't think I fully understand what the ALLOC_STACK_RET is supposed to mean. How can functions return stack-allocated data? Is that not always a stack-use-after-free? E.g.
void* func() {
    char data[SIZE];
    return &data[0];
}
I've confirmed that all changed ExtAPI functions were previously (before #1616) STATIC_OBJECT functions, so I'm not sure which function in ExtAPI is an example of a function SVF would like to handle as ALLOC_STACK_RET? Do you happen to know what the original intention behind #1616 was, i.e., for which ExtAPI function was it introduced?
Some c standard library API like void* alloca(size_t size); can return a stack object which is similar to LLVM's alloca instruction. Hence we may need to keep ALLOC_STACK_RET, isStackObj, is_static_ret methods and so on.

I had another look at extapi.c, I agree that all of the current ALLOC_STACK_RET annotations should be STATIC_OBJECT. But we need to handle each API call and allocate for a new global/static object by changing a new condition here:
else if (SVFUtil::isa<GlobalValue>(llvmValue) || is_static_ret_obj(llvmValue))
{

}
Let me know your thoughts.

If i understand correctly, you proposal is to create a new node for each call to a "static return" function?

We think there are two requirements SVF should fulfil to correctly handle these functions.
Firstly SVF should take into account that all pointers returned from the same function should all alias. I wrote tests for this for the SVF testing benchmark and as expected they failed. The solution proposed by us in #1680 did not work either.
Secondly the nodes that end up in the points-to sets should still map to existing llvm values.
A bit contradictory, but other methods to access all call sites of a function from an SVF node could substitute the second requirement. Perhaps just keeping a map between nodes of static return functions and sets of llvm values of the call site could suffice.

We don't think creating a new node for each call site is the correct approach as it satisfies the second requirement but not the first. All the nodes would still be seen as different nodes and thus not alias, if I'm not mistaken.

yuleisui · 2025-04-25T04:34:52Z

Thanks for taking a look.
I don't think I fully understand what the ALLOC_STACK_RET is supposed to mean. How can functions return stack-allocated data? Is that not always a stack-use-after-free? E.g.
void* func() {
    char data[SIZE];
    return &data[0];
}
I've confirmed that all changed ExtAPI functions were previously (before #1616) STATIC_OBJECT functions, so I'm not sure which function in ExtAPI is an example of a function SVF would like to handle as ALLOC_STACK_RET? Do you happen to know what the original intention behind #1616 was, i.e., for which ExtAPI function was it introduced?
Some c standard library API like void* alloca(size_t size); can return a stack object which is similar to LLVM's alloca instruction. Hence we may need to keep ALLOC_STACK_RET, isStackObj, is_static_ret methods and so on.
I had another look at extapi.c, I agree that all of the current ALLOC_STACK_RET annotations should be STATIC_OBJECT. But we need to handle each API call and allocate for a new global/static object by changing a new condition here:
else if (SVFUtil::isa<GlobalValue>(llvmValue) || is_static_ret_obj(llvmValue))
{

}
Let me know your thoughts.
If i understand correctly, you proposal is to create a new node for each call to a "static return" function?

We think there are two requirements SVF should fulfil to correctly handle these functions. Firstly SVF should take into account that all pointers returned from the same function should all alias. I wrote tests for this for the SVF testing benchmark and as expected they failed. The solution proposed by us in #1680 did not work either. Secondly the nodes that end up in the points-to sets should still map to existing llvm values. A bit contradictory, but other methods to access all call sites of a function from an SVF node could substitute the second requirement. Perhaps just keeping a map between nodes of static return functions and sets of llvm values of the call site could suffice.

We don't think creating a new node for each call site is the correct approach as it satisfies the second requirement but not the first. All the nodes would still be seen as different nodes and thus not alias, if I'm not mistaken.

yuleisui · 2025-04-25T04:41:52Z

@adriaanjacobs

Very good points! We may need two types annotations for distinguish whether a extAPI returns a shared or an individually created object node. SHARED_STATIC_OBJECT or INDIVIDUAL_STATIC_OBJECT

To satisfy your Requirement 1: SHARED_STATIC_OBJECT should be used for each ext function (one llvm function value to one shared static/global object). To satisfy your Requirement 2: for mapping a node back to llvm value, I think we need each SHARED_STATIC_OBJECT to be an individually defined global variable in the extapi.c.

Let me know your thoughts.

I am unsure which test case you mentioned failed. But Requirement 1 does introduce conservative and spurious results.

shuangxiangkan · 2025-04-25T13:58:43Z

In #1693, we removed the "ALLOC_STACK_RET" annotation for APIs that return statically allocated objects. Instead, we now handle these cases by introducing a global variable for each such API, which allows us to model shared static objects. We also retain the "ALLOC_STACK_RET" annotation for APIs that are expected to return a newly allocated object on each call.

@mortkever and @adriaanjacobs, does this work for you? or any other thoughts or ideas?

adriaanjacobs added 3 commits April 9, 2025 17:03

rename ALLOC_STACK_RET -> STATIC_RET

62b4268

bugfix: check for allocainst instead of nonsensical ALLOC_STACK_RET

4d7ece4

update lingering doc string

db3fabe

shuangxiangkan mentioned this pull request Apr 10, 2025

Extapi getenv handling #1680

Open

yuleisui closed this Apr 25, 2025

yuleisui reopened this Apr 25, 2025

mortkever mentioned this pull request Apr 25, 2025

Basic test for some static return functions SVF-tools/Test-Suite#157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix statically allocated objects #1688

Fix statically allocated objects #1688

Fix statically allocated objects #1688

Are you sure you want to change the base?

Fix statically allocated objects #1688

Conversation

Codecov Report