Skip to content

Swapped Identity Assignment in APICreateSocketPair#404

Open
thouravi wants to merge 1 commit intoValveSoftware:masterfrom
thouravi:patch-1
Open

Swapped Identity Assignment in APICreateSocketPair#404
thouravi wants to merge 1 commit intoValveSoftware:masterfrom
thouravi:patch-1

Conversation

@thouravi
Copy link
Copy Markdown
Contributor

pConn[0] is initialized with pIdentity[1] and scopeLock[1]
pConn[1] is initialized with pIdentity[0] and scopeLock[0]

The indices are inverted. Each connection slot gets the wrong identity and the wrong scope lock.

Fix:

pConn[0] = new CSteamNetworkConnectionlocalhostLoopback( pSteamNetworkingSocketsInterface, pIdentity[0], scopeLock[0] );
pConn[1] = new CSteamNetworkConnectionlocalhostLoopback( pSteamNetworkingSocketsInterface, pIdentity[1], scopeLock[1] );

@zpostfacto
Copy link
Copy Markdown
Contributor

I am hesitant to change the behaviour of this function since there might be people using this function that depends on the current behaviour. I am tempted to just update the documentation to match the current behaviour. The current behaviour is confusing, but honestly this is already inherently confusing because when you get info about a connection, you get the REMOTE identity, not the local one. I believe that is why it is swapped, because that is actually less surprising than the non-swapped case. So I think the correct thing to do is just to avoid changing behaviour and risking breaking stuff, and to just update the documentation to clarify the exact behaviour.

Also, the same bug exists in the pipe version, so we would need them to be consistent.

@thouravi
Copy link
Copy Markdown
Contributor Author

That’s a fair point, I hadn’t considered that existing callers might be relying on the current behavior to get the remote identity.

One thing worth considering: could we add a note in the docs flagging this as counterintuitive, so future callers don’t have to discover it the hard way? Maybe even a comment in the source near the assignment.

And Agreed on keeping the pipe version consistent, whatever approach we take here should apply there too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants