Skip to content
This repository has been archived by the owner on May 27, 2021. It is now read-only.

Fixed corruption due to Unicode/ANSI API mismatch #6

Closed
wants to merge 3 commits into from
Closed

Fixed corruption due to Unicode/ANSI API mismatch #6

wants to merge 3 commits into from

Conversation

fatcerberus
Copy link

Have to call WSAAddressToStringA (non-Unicode version) manually, otherwise Visual Studio redefines it to call the wide version, causing a buffer overrun and a corrupted address string.

Bruce E. Pascoe and others added 3 commits March 16, 2015 00:03
Have to call WSAAddressToStringA (non-Unicode version) manually,
otherwise Visual Studio redefines it to call the wide version, causing
corruption and a buffer overrun.
Stupid MSVC autoformatting.
@rxi
Copy link
Owner

rxi commented Aug 9, 2015

I've merged #9 which makes the change from WSAAddressToString() to WSAAddressToStringA().

Is _WINSOCK_DEPRECATED_NO_WARNINGS #define required for you to get a build without warnings (or fewer warnings)? If so can you update this pull request or make a new one such that only that change is included.

You mentioned some other warnings in the comments for #9 -- I should be able to fix the SOCKET casting myself but I'd welcome any commits which fix other warnings on MSVC; I don't have direct access to MSVC myself and can't test things or fix things regarding it easily.

Thanks!

@fatcerberus
Copy link
Author

Hmm, in an older build of dyad, _WINSOCK_DEPRECATED_NO_WARNINGS was indeed necessary to avoid some "unsafe function" warnings related to Winsock calls that weren't caught by _CRT_SECURE_NO_WARNINGS, but whatever changed since then appears to have fixed them.

The SOCKET casting is because SOCKET in Winsock is an intptr_t. I'm honestly not sure how safe it is to blindly cast that to an int--if the implementation of Winsock ever changes such that the SOCKET is an actual pointer value, casting to int (which is 32-bit on windows) will break under x64. Currently it doesn't appear to be an issue, but something to keep in mind.

@rxi
Copy link
Owner

rxi commented Aug 9, 2015

...but whatever changed since then appears to have fixed them.

In that case I'll close this pull request as it doesn't seem relevant any longer.

The SOCKET casting is because SOCKET in Winsock is an intptr_t. I'm honestly not sure how safe it is to blindly cast that to an int

I'll probably introduce a dyad_Socket which typedefs to SOCKET on windows and int everywhere else. I had assumed SOCKET was an int on windows but it seems worth changing given this isn't the case.

Did you get warnings regarding anything else or just the intptr_t conversion?

@rxi rxi closed this Aug 9, 2015
@fatcerberus
Copy link
Author

I noticed a few size_t -> int warnings as well on x64.

@rxi
Copy link
Owner

rxi commented Aug 9, 2015

Ah, I'll have to look into that too.

I committed the change I mentioned above, converting the ints used for sockets to dyad_Socket which is typedefed to SOCKET on Windows; hopefully this fixes the SOCKET cast set of warnings.

@fatcerberus
Copy link
Author

@rxi For reference, here are the warnings I get when compiling dyad.c for x64 with MSVC 2015 as of the latest build (warning level /W4):

1>..\src\dyad.c(60): warning C4244: '=': conversion from 'int' to 'u_short', possible loss of data
1>..\src\dyad.c(558): warning C4456: declaration of 'e' hides previous local declaration
1>  ..\src\dyad.c(518): note: see declaration of 'e'
1>..\src\dyad.c(723): warning C4244: 'function': conversion from 'dyad_Socket' to 'int', possible loss of data
1>..\src\dyad.c(977): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data
1>..\src\dyad.c(1026): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data
1>..\src\dyad.c(1062): warning C4244: '=': conversion from 'int' to 'char', possible loss of data
1>..\src\dyad.c(1066): warning C4244: '=': conversion from 'int' to 'char', possible loss of data

The build I was using before had a few double -> int conversion warnings, so it looks like those were fixed, at least.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants