Remove unsafe access to Socket.Address #93
Conversation
|
I think this might need some review, unfortunately the diff is worse than it needs to be because quite a bit of code just gets indented |
|
Yeah, this’ll take awhile. I’ll get to it as soon as I can. At a quick glance, I like it a lot. I’ve always been a bit leery of these functions and was never quite satisfied. Did you submit a CLA? Unfortunately, I can’t take the contribution without one. |
|
Ok, ill see about the CLA asap. Tomorrow I'll have time to test the changes I've made a bit more thoroughly, I wanted to throw it up here now. I think there are quite a few more manipulations of With this I was trying to write packet to some IP address |
|
Must say, its not very encouraging for the open source nature of the project having to give IBM a license to do what they want with the code. |
|
I know what you mean. It’s the lawyers who’ve gummed up the works. I’m getting ready to retire on November 2 and still plan on maintaining this and a few other repos that I’m responsible for and I found out that after I retire, I’ll need to submit a CLA if I want to keep maintaining them. Go figure!!! 🤣 |
asAddr funcs were passing unsafe pointers outside of the callbacks in which try should be used Using callback to make all the access within these callbacks Fixes bug where IPv6 address was 'cast' to sockaddr but actually was zero'd beyond the length of sockaddr_in and packets written to the wrong address
mcfedr
force-pushed
the
fix-unsafe-socket
branch
from
f0a4e9f
to
2d58a7a
Compare
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
==========================================
+ Coverage 49.31% 50.46% +1.15%
==========================================
Files 3 3
Lines 2610 2473 -137
==========================================
- Hits 1287 1248 -39
+ Misses 1323 1225 -98
Continue to review full report at Codecov.
|
|
@mcfedr Did you decide whether or not to submit a CLA? I've just about completed the review of this PR and anticipate being able to accept it soon providing I have a CLA. Thanks. |
|
Yes. I have sent it in to the contact in the document.
…On Thu, 19 Oct 2017, 22:13 Bill Abt, ***@***.***> wrote:
@mcfedr <https://github.com/mcfedr> Did you decide whether or not to
submit a CLA? I've just about completed the review of this PR and
anticipate being able to accept it soon providing I have a CLA. Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#93 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAq_ZAsOcbgOibgtVYvGdV0CAsgWgAxKks5st59agaJpZM4P1mFP>
.
|
|
@mcfrdr: Can you send me a copy? Apparently he hasn’t received it. Please send to my email address, babt@me.com. Thanks. |
|
Sure, I just forwarded it to you.
…On Fri, 20 Oct 2017 at 16:30 Bill Abt ***@***.***> wrote:
@mcfrdr: Can you send me a copy? Apparently he hasn’t received it. Please
send to my email address, ***@***.*** Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#93 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAq_ZE77eJMTF-96h5vBe_PH6sOE0zIjks5suKCFgaJpZM4P1mFP>
.
|
|
@mcfedr: I've got just one concern with this PR. Maybe there's a way around it, maybe it's the best or only way of doing it. Not sure. The concern is using the exception mechanism to control flow like you do with NotAnError. It's definitely a novel way of doing it but it almost seems abusive of the exception mechanism, no? I've been looking at a better way of doing it but so far have come up blank. Did you also look at alternative ways? The biggest issue I have with using the exception mechanism for normal flow is performance. It's kinda heavy handed. On the plus side, the only time that NotAnError is thrown is when the accept is interrupted so I guess you could call that an exception to the normal flow. If I (or you) can't come up with a better way by Wednesday, I'm going to go with it and do the merge. However, I do request that you change the name of NotAnError to OperationInterrupted or something similar. Makes it clearer to a reader exactly what is going on and why the exception is being thrown. Thanks. |
|
I agree, it felt wrong writing it, but its the only way I could think of to do that early return inside the closures. Your name is much better, I'll definitely make that change. |
|
Ok, cool. I'm in agreement. Make the name change and once it passes CI, I'll do the merge. Thanks. |
|
@mcfedr: Something like this maybe? Then throw it like this... Indicating the accept operation was interrupted. |
|
@mcfedr: If it's ok with you, I'll do the merge and then make the change as I outlined above and then do the release. |
…was included in PR #93. This makes things clearer. Also, made the new exception part of Socket to avoid redundant declarations.
|
Great. I've been traveling a bit right now so haven't had a chance to get
to it. Glad to see you've handled this.
…On Thu, 26 Oct 2017, 14:24 Bill Abt, ***@***.***> wrote:
Merged #93 <#93>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#93 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAq_ZKLO_vJrutt2tCHItx7HuUHQKMF8ks5swIgNgaJpZM4P1mFP>
.
|
Description
asAddr funcs were passing unsafe pointers outside of the callbacks in which try should be used
Using callback to make all the access within these callbacks
Motivation and Context
Fixes bug where IPv6 address was 'cast' to sockaddr but actually was zero'd beyond the length of sockaddr_in and packets written to the wrong address
Checklist: