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.

Sign up for GitHub

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

Jump to bottom

FIX proposal for a crash that happens because of some misuse of const_cast<> #1920

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

FIX proposal for a crash that happens because of some misuse of const_cast<> #1920

wants to merge 2 commits into from

Conversation

Nibelheims
Copy link

Hello,
First of all, thank you for providing libACE.
This commit is a proposal to fix a crash that can happen with ACE::sendv_n_i() and ACE::writev_n(), in case the given iovecs have been allocated on the heap.
Indeed in case of a partial send, libACE will loop until all the data has been sent. The problem is that it updates its arguments when doing so, leaving the caller with one or more updated pointers inside the table of iovecs. Attempting to free such a pointer results in a fault (the value can even not be aligned properly anymore).
The fix simply copies the iovec data if necessary (no perf penalty otherwise), and updates those values instead of the arguments.
Thank you.

@Nibelheims
FIX proposal for a crash that happens when feeding ACE::sendv_n_i and
f196006 
ACE::writev_n with iovecs that have been allocated on the heap.
In case of a partial send, libACE will loop until all the data has been
sent. The problem is that it updates its arguments when doing so,
leaving the caller with one or more updated pointers inside the table of
iovecs. Attempting to free such a pointer results in a fault (the value
can even not be aligned properly).
The fix simply copies the iovec data _if necessary_ (no perf penalty
otherwise), and updates those values instead of the arguments.
@jwillemsen jwillemsen added the needs review Needs to be reviewed label Sep 8, 2022
@Nibelheims
Copy link
Author

Hello,
Thank you for considering this PR. As you can see the modification is small and simple. Do not hesitate to discuss it. I am open to modifications if required.
Thanks and best regards,
Nibelheims

@jwillemsen
Copy link
Member

What is exactly copied, only the iovec array but not the application data it refers to, correct? Any idea to extend one of the existing ACE unit tests as reproducer?

@Nibelheims
Copy link
Author

Nibelheims commented Sep 14, 2022 •
edited

Hello,
Thank you for your reply. Sorry you are right, my phrasing was bad. Yes only the iovec array gets copied, not the data it refers to.
I am quite new to libACE, I actually had to investigate a crash on some application and it turned out that it fell in the "data cannot be sent at once" case. Hence this PR.
That application sends data over the network. I don't know if sending big chunks of data (bigger than the MTU at least) to a socket would be enough to reproduce. The easiest way would be to stub ACE_OS::sendv to make it perform a partial send on the input.
I did this PR on my spare time and, unfortunately, could not look at the testing code (not doing so is bad, sorry about that). I had a bit of trouble compiling, I first had to figure out that MPC (which I was unaware of) was required, then I had some trouble making it generate a Makefile. I finally could compile the shared object successfully, but I did not have enough time to go further.

@Nibelheims
Copy link
Author

Hello,
would you have any update on this PR?
Thanks and best regards,
Nibelheims

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs review Needs to be reviewed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Nibelheims @jwillemsen