Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: no longer using a pointer to a movable stack item #28

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

daladim
Copy link
Collaborator

@daladim daladim commented Sep 9, 2022

We're feeding OpenTraceA with an EVENT_TRACE_LOGFILE, which contains (at least) two pointers:

  • one to the callback, run on every ETW event This one points to a static location in code
  • one to a PVOID Context that Windows will give back as argument of the callback This points at UserTrace.data, which previously lived on the stack. This means that moving the UserTrace (e.g. by returning it in a function result) would break the pointer. This is fixed by putting UserTrace.data on the heap. It is now the responsability of the ferrisetw developer (and not its user, who was unaware of it) to not move it.

This changes the public API, so this means this should be released as a new major number of this crate

Refs #18

We're feeding OpenTraceA with an EVENT_TRACE_LOGFILE, which contains (at least) two pointers:
* one to the callback, run on every ETW event
  This one points to a static location in code
* one to a `PVOID Context` that Windows will give back as argument of the callback
  This points at `UserTrace.data`, which previously lived on the stack.
  This means that moving the UserTrace (e.g. by returning it in a function result) would break the pointer.
  This is fixed by putting UserTrace.data on the heap.
  It is now the responsability of the ferrisetw developer (and not its user, who was unaware of it) to *not* move it.

This changes the public API, so this means this should be released as a new major number of this crate
Copy link
Owner

@n4r1b n4r1b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Is an important change for sure! :)

@daladim daladim merged commit 754e350 into n4r1b:next_major_version Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants