xazax.hun marked an inline comment as done.
xazax.hun added inline comments.
================
Comment at: clang/test/Analysis/fuchsia_handle.cpp:210
+ // Because of arrays, structs, the suggestion is to escape when whe no longer
+ // have any pointer to that symbolic region.
+ if (zx_channel_create(0, get_handle_address(), &sb))
----------------
NoQ wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > This has nothing to do with symbolic regions. We can run into this
> > > > problem even if it's a local variable in the current stack frame:
> > > > ```lang=c++
> > > > void foo() {
> > > > zx_handle_t sa, sb;
> > > > escape(&sb); // Escape *before* create!!
> > > >
> > > > zx_channel_create(0, &sa, &sb);
> > > > zx_handle_close(sa);
> > > > close_escaped();
> > > > }
> > > > ```
> > > >
> > > > The solution that'll obviously work would be to keep track of all
> > > > regions that escaped at least once, and then not even start tracking
> > > > the handle if it's getting placed into a region that causes an escape
> > > > when written into or has itself escaped before, but that sounds like a
> > > > huge overkill.
> > > >
> > > > Lemme think. This sounds vaguely familiar but i can't immediately
> > > > recall what my thoughts were last time i thought about it.
> > > `$ cat test.c`
> > > ```lang=c++
> > > void manage(void **x);
> > > void free_managed();
> > >
> > > void foo() {
> > > void *x;
> > > manage(&x);
> > > x = malloc(1);
> > > free_managed();
> > > }
> > > ```
> > > `$ clang --analyze test.c`
> > > ```lang=c++
> > > test.c:8:3: warning: Potential leak of memory pointed to by 'x'
> > > free_managed();
> > > ^~~~~~~~~~~~~~
> > > 1 warning generated.
> > > ```
> > > Sigh.
> > Oh, I see. Yeah this one will be fun to deal with
> The rules are pretty easy though, right?
> ```lang=c++
> 2680 // A value escapes in four possible cases:
> 2681 // (1) We are binding to something that is not a memory region.
> 2682 // (2) We are binding to a MemRegion that does not have stack storage.
> 2683 // (3) We are binding to a top-level parameter region with a
> non-trivial
> 2684 // destructor. We won't see the destructor during analysis, but
> it's there.
> 2685 // (4) We are binding to a MemRegion with stack storage that the store
> 2686 // does not understand.
> 2687 ProgramStateRef
> 2688 ExprEngine::processPointerEscapedOnBind(ProgramStateRef State, SVal
> Loc,
> 2689 SVal Val, const
> LocationContext *LCtx) {
> ```
> Basically, locals are the only special case; writing into anything else
> should be an immediate escape.
>
> We could easily update this procedure to additionally keep track of all
> escaped locals in the program state, and then escape all binds to previously
> escaped locals as well.
>
> The checker would then have to follow the same rules.
>
> In the worst case, manually.
>
> But i think we should instead update the `checkPointerEscape()` callback to
> escape the values of out-parameters upon evaluating the call conservatively
> (if it doesn't already) and then teach the checker to mark escaped regions
> explicitly as escaped (as opposed to removing them from the program state),
> and then teach it to never transition from escaped to opened. That would be
> cleaner because that'll only require us to hardcode the escaping procedure
> once.
>
> Or we could just make the "bool doesWriteIntoThisRegionCauseAnEscape?(Region,
> State)" function re-usable.
Yeah. I wonder if this procedure is the right place though. We do not actually
see a bind in the code above.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71041/new/
https://reviews.llvm.org/D71041
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
