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))
----------------
xazax.hun wrote:
> 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.
Hmm, in the stack example we do see the point of invalidation (which results in
an escape). That make things easier, checkers could even work that problem
around if they wanted to. In the original example, however, there is no point
of invalidation, the region we get is already escaped without seeing any bind
later on. And there is no store into the escaped region, since
`zx_channel_create` is evaluated conservatively, we just attach the state to
the conjured symbol retroactively.
So at this point I wonder if the checker should ever track any symbols that are
is bound to a non-stack region (even if we do not see the bind itself). This
might circumvent most of our problems?
And for the stack example we can just make the escaped state explicit again and
never transition from an escaped state to a non-escaped one.
WDYT?
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
