This is an archive of the discontinued LLVM Phabricator instance.

Support struct annotations in FuchsiaHandleChecker.
ClosedPublic

Authored by aabbaabb on Nov 10 2020, 6:05 PM.

Details

Summary

Support adding handle annotations to sturucture that contains handles.
All the handles referenced by the structure (direct value or ptr) would
be treated as containing the release/use/acquire annotations directly.

This could be used to capture more error cases for example use after
free:

void spawn(ZX_HANDLE_RELEASE const fdio_spawn_action_t* actions);

int main() {
  zx_handle_t handle;
  get_handle(&handle);
  fdio_spawn_action actions[1] = {};
  actions[0].h.handle = handle;
  spawn(actions);
  use_handle(handle);
  return 0;
}

In this example, spawn would release the handle so using the handle
again should be considered an error. Before this patch, handle would
be considered escaped and thus using it again is not reported.

Diff Detail

Event Timeline

aabbaabb created this revision.Nov 10 2020, 6:05 PM
aabbaabb requested review of this revision.Nov 10 2020, 6:05 PM
phosek added inline comments.Nov 11 2020, 12:27 AM
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
231

Can you move this to the private section below?

233
236
238

No need for { and } around a block with a single statement.

250
263

This variable shouldn't be needed at all.

268–269
273
280–281
312

This can be a SmallVector.

378

Ditto, this can be a SmallVector.

520

Ditto, this can be a SmallVector.

aabbaabb updated this revision to Diff 305019.Nov 12 2020, 8:46 PM
aabbaabb marked 12 inline comments as done.
xazax.hun edited the summary of this revision. (Show Details)Nov 13 2020, 7:05 AM
xazax.hun requested changes to this revision.Nov 13 2020, 7:17 AM

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
266

I wonder whether this will actually pick up handles outside of the structure. I.e. when the structure has a pointer field to a handle instead of the handle directly. What is the desired result in that case?

This revision now requires changes to proceed.Nov 13 2020, 7:17 AM

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

aabbaabb added inline comments.Nov 13 2020, 5:28 PM
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
266

In my local test, it is smart enough to find even double pointers (**zx_handle_t) inside the structure.

aabbaabb marked an inline comment as done.Nov 13 2020, 5:28 PM

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

We have the tests here: https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/fuchsia_handle.cpp
In LLVM, we have a very strong requirement to commit tests together with features (unless it is really hard to devise a test).
As a result, you can almost always find the location of the tests by looking at the commit that created the file or introduced the feature.

xazax.hun added inline comments.Nov 16 2020, 2:30 AM
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
266

This is great, but I think that contradicts the comment above: handle inside the structure.
One could argue, the handle in this case is not inside.

I think you should change to comment to reflect this and make sure this is the desired behavior. I.e. I want to know whether this is what the users would intuitively expect and want.

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

We have the tests here: https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/fuchsia_handle.cpp
In LLVM, we have a very strong requirement to commit tests together with features (unless it is really hard to devise a test).
As a result, you can almost always find the location of the tests by looking at the commit that created the file or introduced the feature.

Thanks, I didn't find it. Added the tests.

aabbaabb updated this revision to Diff 305635.Nov 16 2020, 6:28 PM
aabbaabb edited the summary of this revision. (Show Details)
aabbaabb added a comment.EditedNov 18 2020, 1:54 AM

I have a question about pointer members and I'd like to see some tests added. Otherwise, I like the direction.

Where should i add the tests?

We have the tests here: https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/fuchsia_handle.cpp
In LLVM, we have a very strong requirement to commit tests together with features (unless it is really hard to devise a test).
As a result, you can almost always find the location of the tests by looking at the commit that created the file or introduced the feature.

I also have some questions:

  1. This patch does not work for case where the structure is passed in as pointer. The structure becomes lazycompoundval and I could not find the symbol there. Do you know what is going on?
  2. If the structure contains a union, the escape logic could not correctly find the escaped handle:
struct HandleStruct {
  zx_handle_t handle;
  union {
    struct {
      zx_handle_t handle;
    } h;
  };
};

zx_handle_t handle;
get_handle(&handle);
HandleStruct hs = {
  .handle = handle
};
// Call this function could escape the handle. 
call_function(hs);

If you change the code to:

HandleStruct hs = {
  .h.handle = handle;
}
// Call this function would not escape the handle. The handle is not in the escaped array. 
call_function(hs);

If you change it to:

HandleStruct hs;
hs.h.handle = handle;
// Call this function would escape handle even though this should be the same as the previous case.
call_function(hs);

This is causing false positives where handle are actually escaped but considered as leaked. Do you know why?

I also have some questions:

  1. This patch does not work for case where the structure is passed in as pointer. The structure becomes lazycompoundval and I could not find the symbol there. Do you know what is going on?

Invoking scanReachableSymbols on the pointer itself would also not work?

  1. If the structure contains a union, the escape logic could not correctly find the escaped handle:

Unfortunately, union support is rather incomplete in the analyzer. There could be many problems in code relying on unions. In case you want to solve this problem it is very likely that you will need to improve the analyzer itself. You might be able to add workarounds in the checker to avoid false positives, like stop following symbols after it was stored into a union eagerly.

clang/test/Analysis/fuchsia_handle.cpp
2 ↗(On Diff #305635)

Is this change intended?

aabbaabb added a comment.EditedNov 18 2020, 12:49 PM

I also have some questions:

  1. This patch does not work for case where the structure is passed in as pointer. The structure becomes lazycompoundval and I could not find the symbol there. Do you know what is going on?

Invoking scanReachableSymbols on the pointer itself would also not work?

  1. If the structure contains a union, the escape logic could not correctly find the escaped handle:

Unfortunately, union support is rather incomplete in the analyzer. There could be many problems in code relying on unions. In case you want to solve this problem it is very likely that you will need to improve the analyzer itself. You might be able to add workarounds in the checker to avoid false positives, like stop following symbols after it was stored into a union eagerly.

Thanks.

  1. Yes, that would also not work. I have dumped the visited symbols and I see a (conj_$3, int, XXX) symbol which I do not understand where does it come from, the handle symbol should be (conj_$1, zx_handle_t, XXX):
typedef uint32_t zx_handle_t;

void get_handle(ZX_HANDLE_ACQUIRE zx_handle_t* handle);

void release_handle_struct(ZX_HANDLE_RELEASE HandleStruct* s);

int main() {
  zx_handle_t handle;
  get_handle(&handle);
  HandleStruct action {
    .handle = handle,
    .a = 10
  };
  release_handle_struct(&action);
  return 0;
}

I use this code to test and the getFuchsiaHandles post the release_handle_struct call is unable to find the handle symbol.

  1. Okay, if that's the case, I would ignore the union case for now.

I think I might have an idea of what is going on. The handles are released in a PostCall. This is intended, as the handles are in the released state AFTER the call. When you call an unknown function with a pointer, that function is free to modify the contents of the pointer. Since the analyzer has no knowledge about the body of the function, it will do a conservative evaluation, escape the symbols in the struct, and create new symbols. This is why new symbols might be introduced. I think you could subscribe to escapes and check if you access the old symbols there. If that is the case and the escape is the result of calling an annotated function, maybe you can correctly mark those symbols as deleted.

I think I might have an idea of what is going on. The handles are released in a PostCall. This is intended, as the handles are in the released state AFTER the call. When you call an unknown function with a pointer, that function is free to modify the contents of the pointer. Since the analyzer has no knowledge about the body of the function, it will do a conservative evaluation, escape the symbols in the struct, and create new symbols. This is why new symbols might be introduced. I think you could subscribe to escapes and check if you access the old symbols there. If that is the case and the escape is the result of calling an annotated function, maybe you can correctly mark those symbols as deleted.

Thanks, i would try to improve that in a separate patch. Do you have any more comments on this CL? Could you +2 this one?

xazax.hun accepted this revision.Nov 19 2020, 9:03 PM

Overall looks good, but please resolve the two nits before commiting.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
266

The comment above is not updated yet.

clang/test/Analysis/fuchsia_handle.cpp
2 ↗(On Diff #305635)

I think we don't need this change.

This revision is now accepted and ready to land.Nov 19 2020, 9:03 PM
aabbaabb updated this revision to Diff 306778.Nov 20 2020, 2:07 PM
aabbaabb marked 4 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptNov 21 2020, 8:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@haowei https://llvm.org/docs/DeveloperPolicy.html#commit-messages Please use git commit --amend --author for future patches where the majority of the work is done by others.

@haowei https://llvm.org/docs/DeveloperPolicy.html#commit-messages Please use git commit --amend --author for future patches where the majority of the work is done by others.

Thanks for your remind.