-
-
Notifications
You must be signed in to change notification settings - Fork 726
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 Issue 20544 - socket.remoteAddress throws out of memory error with unix domain socket peer #7383
Conversation
Thanks for your pull request and interest in making D better, @adamdruppe! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#7383" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, can you adda changelog entry or reference a bug report please.
I suspect the root cause is a bit deeper here but my test case revealed it buried in thousands of lines of code and I haven't fully reduced it yet. My feeling right now is the constructor on line 2011 is actually setting the length wrong given the abstract linux socket (which starts with a nul byte, but then has other data after that). I'll probably come back tomorrow and hopefully follow up with more detail and perhaps do an additional PR. But I figured might as well open this since regardless of how the state got here, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can see, the only situation in which this change would have an effect, is when the socket is constructed with an empty path. This is obviously not going to do anything useful, so I think the correct fix is to add an enforce
to the constructor to check for that. (The logic in the constructor is already wrong for the case of empty paths, as the abstract UNIX socket check then references an out-of-bounds character.)
I would rather say that the problem is that if |
So what specifically caused this in my code is a call to toString does appear to work with abstract names set with the other constructor, and it appears to to work with strings of the maximum length. So I believe this protected ctor is the problem. It should probably at least null terminate it lol. Maybe fill with zeroes instead of '?'. BTW though in my code I just tested with a cast; |
I think that constructor call should be immediately followed by a Could you please have a look at what exactly is going on, or at least, provide a minimal reproducible testcase? |
yeah im just looking at the commit history to see why it is doing that ? thing right now anyway. git blame led here: https://github.com/dlang/phobos/pull/3300/files which seems to be just changing from dynamic to static allocation so i don't think changing the ctor will hurt anything. Here's a little program that reproduces (on Linux)
Note the abstract socket is not necessary, it works the same way with a regular name too. |
so getpeername returns 2 with an unbound unix domain socket, which means the sun_path is entirely untouched as that means only the family tag (a ushort) is initiated. Which means So yeah this change is actually the correct solution as there is no deeper root cause. Name length is simply allowed to be zero. refs: |
OK. Agreed, except I think it would be better to explicitly return |
yea i'll make that change, force pushed up now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should reference a bugzilla issue.
std/socket.d
Outdated
@@ -2024,6 +2024,8 @@ static if (is(sockaddr_un)) | |||
@property string path() @trusted const pure | |||
{ | |||
auto len = _nameLen - sockaddr_un.init.sun_path.offsetof; | |||
if (len == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No tabs.
if (len == 0) | |
if (len == 0) |
std/socket.d
Outdated
@@ -2024,6 +2024,8 @@ static if (is(sockaddr_un)) | |||
@property string path() @trusted const pure | |||
{ | |||
auto len = _nameLen - sockaddr_un.init.sun_path.offsetof; | |||
if (len == 0) | |||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment would be good.
return null; | |
return null; // An empty path may be returned from getpeername |
Can you add it as a unittest? |
I just added one assert to the existing unittest to call the method since the setup is basically the same anyway. also formally filed: https://issues.dlang.org/show_bug.cgi?id=20544 |
Hmmm the autotester failed on BSD. It is possible that system does different behavior. It is also possible that Linux will change its behavior at some point in the future; I don't think the spec requires it to return null, just that it can. but i could always make it |
yeah im just gonna call the method inside the unittest to ensure it doesn't throw. the comment will explain it "may". |
I think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased and fixed the commit message for you.
cool |
eeek dub down again and it seems to cause the buildkite fail. i think. |
Thi fails on circle which is required. |
Is there a way to tell it to try again? I don't see any legitimate reason for this to fail. |
Force push, unless you have permissions to retrigger builds. |
…h unix domain socket peer
yeah there we go it passed this time. I suspect the problems yesterday were actually just dub's 502 rather than something related here. |
i think the force push disabled the auto-merge flag tho |
No description provided.