Closed Bug 951677 Opened 6 years ago Closed 5 years ago

Support bind in SocketTransport

Categories

(Core :: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: kk1fff, Assigned: kk1fff)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 6 obsolete files)

To support bind() in TCPSocket in content process, we need to add bind() function in SocketTransport. (or in SocketTransportService)
Flags: needinfo?(honzab.moz)
I think this can be exposed on nsISocketTransport.  Get inspired by nsIUDPSocket.idl how to pass an address (mozilla::net::NetAddr).  The new nsISocketTransport.bind(in NetAddr aAddr) must only be called called before opening streams on the transport (i.e. before PR_Connect call).  Otherwise it must/should fail.

Please be aware of bug 444328 and patch https://bugzilla.mozilla.org/attachment.cgi?id=8346933&action=edit.  Please base your work on that patch, otherwise Steve may not like you ;)  That patch is already r+'ed and soon should land anyway.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #1)
> Please be aware of bug 444328 and patch
> https://bugzilla.mozilla.org/attachment.cgi?id=8346933&action=edit.  Please
> base your work on that patch, otherwise Steve may not like you ;)  

Yup, please stay in sync with the status of that bug :)

> That patch is already r+'ed and soon should land anyway.

The first patch has landed; two more are in review. The first one is most important: it adds PRFileDescAutoLock which you should use to get access to the PRFileDesc in a function. The second and third patches are also important, but are probably more isolated from your work. Still, keep me updated please and let me know (needinfo me) if you have any problems.
Thank for the suggestion!

I have another question: PR_Connect is called automatically after SocketTransportService::CreateTransport(), is there a chance to call Bind() after SocketTransport is created and before its socket is connected? Or we should create SocketTransport with new when we want to bind to an address?
Flags: needinfo?(honzab.moz)
(In reply to Patrick Wang [:kk1fff] from comment #3)
> Thank for the suggestion!
> 
> I have another question: PR_Connect is called automatically after
> SocketTransportService::CreateTransport(), 

It's definitely not.  It actually cannot be.

> is there a chance to call Bind()
> after SocketTransport is created and before its socket is connected? Or we
> should create SocketTransport with new when we want to bind to an address?

InitiateSocket() where from PR_Connect is called cannot fire sooner then you open output or input stream.  Just bind before a stream is open.  If already done so, throw.  See comment 1.
Flags: needinfo?(honzab.moz)
Thanks for clarification, Honza!
Rebasing.
Attachment #8357808 - Attachment is obsolete: true
Assignee: nobody → kk1fff
Attachment #8463777 - Flags: review?(honzab.moz)
Comment on attachment 8463777 [details] [diff] [review]
Patch: add Bind function to SocketTransport.

Review of attachment 8463777 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +2218,5 @@
> +{
> +    NS_ENSURE_ARG(aLocalAddr);
> +
> +    if (mFDconnected) {
> +        return NS_ERROR_FAILURE;

I think mAttached is a better check.  You should not be able to change this after PR_Connect is called.  However, mFDconnected means we've reached the peer and that could be racy.
Attachment #8463777 - Flags: review?(honzab.moz) → feedback+
Attached patch Proposed Patch (obsolete) — Splinter Review
Check mAttached before binding to a local address.
Attachment #8463777 - Attachment is obsolete: true
Attachment #8490612 - Flags: review?(honzab.moz)
Comment on attachment 8490612 [details] [diff] [review]
Proposed Patch

Review of attachment 8490612 [details] [diff] [review]:
-----------------------------------------------------------------

r- for a missing test.  Please write something that at least exercises the code, best to also check at least the port number is as expected.  This way it seems like you've never run it at all.  However, if you find (or argument) the test would be hard to write (since to find the right port number that will not in 100% cases collide), we can omit it.  But that research first needs to be done.
Attachment #8490612 - Flags: review?(honzab.moz) → review-
Attached patch Test case (obsolete) — Splinter Review
Test binding a port. To avoid binding to a port that is already used, it retries with next port when input stream gets error, until test server received connection or the retry limit is reached.
(In reply to Honza Bambas (:mayhemer) from comment #10)
> r- for a missing test.  Please write something that at least exercises the
> code, best to also check at least the port number is as expected.  This way
> it seems like you've never run it at all.  However, if you find (or
> argument) the test would be hard to write (since to find the right port
> number that will not in 100% cases collide), we can omit it.  But that
> research first needs to be done.

I used to think that write a test for bind is not easy, since there may need many interfaces to be on the test machine to tell if we bound to an interface successfully. But as you've mentioned, binding to a port can also be a way to implement the test case. I've added a test case for this. Thanks.
Attached patch Test case v.2 (obsolete) — Splinter Review
Fix build failure on try.
Attachment #8494374 - Attachment is obsolete: true
Attachment #8494936 - Flags: review?(honzab.moz)
Attachment #8490612 - Flags: review- → review?(honzab.moz)
Attached patch Test case v.3Splinter Review
Remove unnecessary header files.
Attachment #8494936 - Attachment is obsolete: true
Attachment #8494936 - Flags: review?(honzab.moz)
Attachment #8495041 - Flags: review?(honzab.moz)
Attached patch Proposed Patch (obsolete) — Splinter Review
Using an auto pointer to hold the NetAddr that holds the address we are going to bind. I just realized how large a NetAddr is, as Bind() won't be used by many people, it will be an unnecessary waste. (And leak detector treat the increment of memory size as a leak.)
Attachment #8490612 - Attachment is obsolete: true
Attachment #8490612 - Flags: review?(honzab.moz)
Attachment #8499302 - Flags: review?(honzab.moz)
Can you explain why you need the lock over mBindAddress now?
Comment on attachment 8495041 [details] [diff] [review]
Test case v.3

Review of attachment 8495041 [details] [diff] [review]:
-----------------------------------------------------------------

Cool!  Have you tested the retry logic carefully?  I'll leave this up to you.
Attachment #8495041 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #16)
> Can you explain why you need the lock over mBindAddress now?

Yes. Since it looks like InitiateSocket() is called on STS, while Bind() is called on main thread.
(In reply to Honza Bambas (:mayhemer) from comment #17)
> Have you tested the retry logic carefully?  I'll leave this up to you.

Yes, I've tested this by binding to a port that is already in use. And it will bind to next port.
Comment on attachment 8499302 [details] [diff] [review]
Proposed Patch

Review of attachment 8499302 [details] [diff] [review]:
-----------------------------------------------------------------

Few nits, I'll check quickly on the next version.  Thanks!

::: netwerk/base/public/nsISocketTransport.idl
@@ +56,5 @@
>       */
>      [noscript] NetAddr getSelfAddr();
>  
>      /**
> +     * Bind to specific local address.

to a specfic

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +1356,5 @@
>      //
>      PRNetAddr prAddr;
> +    {
> +        MutexAutoLock lock(mLock);
> +        if (mBindAddr) {

to optimize this a bit, check mBindAddr being non-null outside the lock, but work with it under the lock.  Like:

if (mBindAddr) {
  lock(mLock);
  ...
  PR_Bind(..);
  mBindAddr = nullptr;
}

@@ +2237,5 @@
> +nsSocketTransport::Bind(NetAddr *aLocalAddr)
> +{
> +    NS_ENSURE_ARG(aLocalAddr);
> +
> +    if (mAttached) {

check this also under the lock.
Attachment #8499302 - Flags: review?(honzab.moz) → feedback+
Attached patch Proposed PatchSplinter Review
Revised.
Attachment #8499302 - Attachment is obsolete: true
Attachment #8500287 - Flags: review?(honzab.moz)
Comment on attachment 8500287 [details] [diff] [review]
Proposed Patch

Review of attachment 8500287 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  r=honzab.
Attachment #8500287 - Flags: review?(honzab.moz) → review+
Thank you for review, Honza!
https://hg.mozilla.org/mozilla-central/rev/8d52ce400c01
https://hg.mozilla.org/mozilla-central/rev/27626f68637d
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

If you need this for tcp sockets only it is fine, if you want to use it for a "ssl" socket it will not work.
I will fix this as a part of another bug. This is just a remark since bind function is exposed in nsISocketTransport and it can be an "ssl" socket as well.
Flags: needinfo?(kk1fff)
I see. Thank you for finding and fixing the bug. Dragana. Just curious, since this patch is just done recently, is there already a an SSL socket working with bind()?
Flags: needinfo?(kk1fff)
I posted a new bug, because the other bug I was talking about probably will take longer to lend
The new bug is bug 1087213.
Blocks: 1039655
No longer depends on: 1039655
No longer blocks: 950660
Depends on: 950660
No longer blocks: 1039655
You need to log in before you can comment on or make changes to this bug.