1372246 - Fix the address size for bind in _PR_MD_TCPSENDTO and return error for GetOverlappedResult

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement)

Description

[Dragana Damjanovic [:dragana]]

Attachment: bug_fixTFO_bind.patch

Comment 1

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c76ec9b048f14e602d66a6858d263e9d7cddfa58

Comment 2

[Dragana Damjanovic [:dragana]]

I do not think this cause crashes, but anyway need to be fixed.

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Dragana Damjanovic [:dragana] from comment #2)
> I do not think this cause crashes

:(

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8876751 [details] [diff] [review]
bug_fixTFO_bind.patch

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

this makes sense to me - but kai is the new nspr owner..

Comment 5

[Kai Engert [:KaiE:]]

Could you please briefly explain the motivation for this change? Might be helpful in the future if someone comes back here and wants to learn about the history.

Regarding the change of parameters for the bind call:
Changing the second parameter seems unnecessary, because bindAddr is a union, the old address of the variable should be the same as the address of the first member or the second member, right?

I assume the change to the third parameter was a major motivation for this patch, because PR_NETADDR_SIZE can apparently create a more complex result than a simple sizeof?

I don't know the lowlevel details of this code, so Patrick's r+ is enough.

Comment 6

[Dragana Damjanovic [:dragana]]

(In reply to Kai Engert (:kaie:) from comment #5)
> Could you please briefly explain the motivation for this change? Might be
> helpful in the future if someone comes back here and wants to learn about
> the history.
> 
> Regarding the change of parameters for the bind call:
> Changing the second parameter seems unnecessary, because bindAddr is a
> union, the old address of the variable should be the same as the address of
> the first member or the second member, right?
> 
> I assume the change to the third parameter was a major motivation for this
> patch, because PR_NETADDR_SIZE can apparently create a more complex result
> than a simple sizeof?
> 
> I don't know the lowlevel details of this code, so Patrick's r+ is enough.

The bind parameters:
The change on the second parameter does not actually change anything. i made this change only to make this call similar to the other calls that have PRNetAddr as a parameter, just that.

The second change to bind: the address length was wrong, e.g. the address length of ipv4 is smaller than for ipv6. The windows bind function should do the right thing even if the length is wrong, it should check for address family and decide what data it needs. But to make this correct I have change the value.

I thought that GetOverlappedResult returns ERROR_IO_PENDING when ConnectEx has not finished yet, but it does not it return ERROR_IO_INCOMPLETE. In the Firefox case we use polling to check when socket is read, therefore I assume that we will never hit this error, but in case some uses a blocking socket or uses nspr in a different way it can hit this bug.

Comment 7

[Kai Engert [:KaiE:]]

Thanks for the explanations. In which version of Firefox will you require this change? Will Firefox 56 be sufficiently early?

Comment 8

[Dragana Damjanovic [:dragana]]

56 is just fine.

Thanks a lot!

Comment 9

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nspr/rev/24f9bb30f9f2

We need to change the target to 4.16 when bug 1370873 is fixed.