-
Notifications
You must be signed in to change notification settings - Fork 80
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 some x64 compatibility issues #873
Conversation
These only produce warnings, not errors.
Pointer math that really really assumes 32-bit addresses 😬 |
This was due to treating a <signed> * <unsigned> multiplication as unsigned, which worked in a 32-bit address space due to overflow wraparound, but goes way off into uncharted memory in a 64-bit address space.
@@ -92,7 +92,7 @@ void plDispatchLog::LogStatusBarChange(const char* name, const char* action) | |||
memset(&mbi, 0, sizeof(MEMORY_BASIC_INFORMATION)); | |||
|
|||
// Note: this will return shared mem too on Win9x. There's a way to catch that, but it's too slow -Colin |
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.
Who cares lol
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.
@@ -1128,7 +1128,7 @@ void plFont::IRenderChar8To32AlphaPremShadow( const plFont::plCharacter &c ) | |||
y = fRenderInfo.fClipRect.fY - fRenderInfo.fY + (int16_t)c.fBaseline; | |||
if( y < -2 ) | |||
y = -2; | |||
destBasePtr = (uint32_t *)( (uint8_t *)destBasePtr + y*fRenderInfo.fDestStride ); | |||
destBasePtr = (uint32_t *)((uint8_t *)destBasePtr + y * int32_t(fRenderInfo.fDestStride)); |
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.
Is this the only spot this happens, or does it do this same calculation for each of the different rendering types?
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.
It's the only one I found that had an explicit negative value, but I did find another potentially negative multiplier (just pushed)... The rest are either already checked for negative or already converted to signed.
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.
IIRC This function was written by @cwalther to support text shadowing in the KI.
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.
Right – and I had probably just forgotten that int16_t * uint32_t = uint32_t at that point. The code was copied from other functions, but this is the only one where y can be negative (because shadows can extend beyond the top of the character).
What about line 367? A quick glance around line 266 suggests that fRenderInfo.fY
could be negative. That would make fDestPtr
go out of bounds, but further arithmetic might bring it back into bounds later before being dereferenced.
Curious that this worked in Win32, but is rejected by x64...
@@ -653,7 +653,8 @@ static void SaveUserPass(LoginDialogParam* pLoginParam, wchar_t* password) | |||
HKEY hKey; | |||
RegCreateKeyEx(HKEY_CURRENT_USER, ST::format("Software\\Cyan, Inc.\\{}\\{}", plProduct::LongName(), GetServerDisplayName()).c_str(), 0, nullptr, REG_OPTION_NON_VOLATILE, KEY_WRITE, nullptr, &hKey, nullptr); | |||
RegSetValueExW(hKey, L"LastAccountName", 0, REG_SZ, (LPBYTE) pLoginParam->username, sizeof(pLoginParam->username)); | |||
RegSetValueEx(hKey, "RememberPassword", 0, REG_DWORD, (LPBYTE) &(pLoginParam->remember), sizeof(LPBYTE)); |
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.
Nice, sizeof(LPBYTE) surely changes in x64, and a DWORD is not of length 8.
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.
I suspect this was originally written directly from MSDN reference code.
@@ -1128,7 +1128,7 @@ void plFont::IRenderChar8To32AlphaPremShadow( const plFont::plCharacter &c ) | |||
y = fRenderInfo.fClipRect.fY - fRenderInfo.fY + (int16_t)c.fBaseline; | |||
if( y < -2 ) | |||
y = -2; | |||
destBasePtr = (uint32_t *)( (uint8_t *)destBasePtr + y*fRenderInfo.fDestStride ); | |||
destBasePtr = (uint32_t *)((uint8_t *)destBasePtr + y * int32_t(fRenderInfo.fDestStride)); |
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.
IIRC This function was written by @cwalther to support text shadowing in the KI.
If you stub out font rendering, it is possible to link to Personal with an x64 build with these changes. I haven't figured out why font rendering explodes in x64 yet, unfortunately :/It's now possible to link to Relto (and a few other ages I tried) without modification. Is this sufficient to close #34? Probably not if you include Max plugin support...