Skip to content

Commit

Permalink
refactor sqMessageBox to void using toUnicode
Browse files Browse the repository at this point in the history
Why to get rid of toUnicode fromUnicode fromSqueak fromSqueak2?

- we'd rather use UTF8 everywhere.
- and it's the last usage remaining!

Now the clients must use a TEXT() macro also for the format. In the future, we could eventually translate the VM messages...

Get rid of wsprintf variant which has no character limit and might be prone to buffer overrun.
An alternative for supporting both ASCII and WIDE is the _tcs* family in <tchar.h>
See https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/vsprintf-vsprintf-l-vswprintf-vswprintf-l-vswprintf-l?view=vs-2017
In order to avoid buffer overrun, prefer a vsnprintf variant

Note: I did also update the prototype in sqWin32SpurAlloc, but not the contents.
It is dead code, and only there for testing purposes.
We'd better remove it!
  • Loading branch information
nicolas-cellier-aka-nice committed Dec 30, 2018
1 parent 98fc85d commit 4f6f191
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 22 deletions.
2 changes: 1 addition & 1 deletion platforms/win32/plugins/FilePlugin/sqWin32FilePrims.c
Expand Up @@ -517,7 +517,7 @@ size_t sqImageFileRead(void *ptr, size_t sz, size_t count, sqImageFile h)
ReadFile((HANDLE)(h-1), (LPVOID) ptr, count*sz, &dwReallyRead, NULL);
while(dwReallyRead != (DWORD)(count*sz)) {
DWORD err = GetLastError();
if(sqMessageBox(MB_ABORTRETRYIGNORE, TEXT("Squeak Warning"),"Image file read problem (%d out of %d bytes read)", dwReallyRead, count*sz)
if(sqMessageBox(MB_ABORTRETRYIGNORE, TEXT("Squeak Warning"),TEXT("Image file read problem (%d out of %d bytes read)"), dwReallyRead, count*sz)
== IDABORT) return (dwReallyRead / sz);
sqImageFileSeek(h, position);
ReadFile((HANDLE)(h-1), (LPVOID) ptr, count*sz, &dwReallyRead, NULL);
Expand Down
2 changes: 1 addition & 1 deletion platforms/win32/vm/sqWin32.h
Expand Up @@ -382,7 +382,7 @@ TCHAR *lstrrchr(TCHAR *source, TCHAR c);
/* Output stuff */
/******************************************************/
#ifndef sqMessageBox
int __cdecl sqMessageBox(DWORD dwFlags, const TCHAR *titleString, const char* fmt, ...);
int __cdecl sqMessageBox(DWORD dwFlags, const TCHAR *titleString, const TCHAR* fmt, ...);
#endif

#ifndef warnPrintf
Expand Down
4 changes: 2 additions & 2 deletions platforms/win32/vm/sqWin32Alloc.c
Expand Up @@ -76,15 +76,15 @@ void *sqAllocateMemory(usqInt minHeapSize, usqInt desiredHeapSize)
} while(!pageBase);
if(!pageBase) {
sqMessageBox(MB_OK | MB_ICONSTOP, TEXT("VM Error:"),
"Unable to allocate memory (%d bytes requested)",
TEXT("Unable to allocate memory (%d bytes requested)"),
maxReserved);
return pageBase;
}
/* commit initial memory as requested */
commit = nowReserved;
if(!VirtualAlloc(pageBase, commit, MEM_COMMIT, PAGE_READWRITE)) {
sqMessageBox(MB_OK | MB_ICONSTOP, TEXT("VM Error:"),
"Unable to commit memory (%d bytes requested)",
TEXT("Unable to commit memory (%d bytes requested)"),
commit);
return NULL;
}
Expand Down
4 changes: 2 additions & 2 deletions platforms/win32/vm/sqWin32Main.c
Expand Up @@ -1424,7 +1424,7 @@ sqImageFile findEmbeddedImage(void) {
sqImageFileSeek(f, endMarker);
sqImageFileRead(&magic, 1, 4, f);
sqImageFileRead(&start, 1, 4, f);
sqMessageBox(MB_OK, "Magic number", "Expected:\t%x\nFound:\t\t%x", SQ_IMAGE_MAGIC, magic);
sqMessageBox(MB_OK, TEXT("Magic number", "Expected:\t%x\nFound:\t\t%x"), SQ_IMAGE_MAGIC, magic);
/* Magic number must be okay and start must be within executable boundaries */
if(magic != SQ_IMAGE_MAGIC || start < 0 || start >= endMarker) {
/* nope */
Expand All @@ -1434,7 +1434,7 @@ sqImageFile findEmbeddedImage(void) {
/* Might have an embedded image; seek back and double check */
sqImageFileSeek(f,start);
sqImageFileRead(&magic, 1, 4, f);
sqMessageBox(MB_OK, "Magic number", "Expected:\t%x\nFound:\t\t%x", SQ_IMAGE_MAGIC, magic);
sqMessageBox(MB_OK, TEXT("Magic number", "Expected:\t%x\nFound:\t\t%x"), SQ_IMAGE_MAGIC, magic);
if(magic != SQ_IMAGE_MAGIC) {
/* nope */
sqImageFileClose(f);
Expand Down
14 changes: 7 additions & 7 deletions platforms/win32/vm/sqWin32SpurAlloc.c
Expand Up @@ -50,7 +50,7 @@ sqAllocateMemory(usqInt minHeapSize, usqInt desiredHeapSize)

if (pageSize) {
sqMessageBox(MB_OK | MB_ICONSTOP, TEXT("VM Error:"),
"sqAllocateMemory already called");
TEXT("sqAllocateMemory already called"));
exit(1);
}

Expand All @@ -77,7 +77,7 @@ sqAllocateMemory(usqInt minHeapSize, usqInt desiredHeapSize)
if (!alloc) {
exit(errno);
sqMessageBox(MB_OK | MB_ICONSTOP, TEXT("VM Error:"),
"sqAllocateMemory: initial alloc failed!\n");
TEXT("sqAllocateMemory: initial alloc failed!\n"));
exit(1);
}
return alloc;
Expand Down Expand Up @@ -114,7 +114,7 @@ address_space_used(char *address, usqInt bytes)
return 1;
if (!VirtualQuery(address, &info, sizeof(info)))
sqMessageBox(MB_OK | MB_ICONSTOP, TEXT("VM Error:"),
"Unable to VirtualQuery range [%p, %p), Error: %u",
TEXT("Unable to VirtualQuery range [%p, %p), Error: %u"),
address, (char *)address + bytes, GetLastError());

addressSpaceUnused = info.BaseAddress == address
Expand Down Expand Up @@ -160,7 +160,7 @@ sqAllocateMemorySegmentOfSizeAboveAllocatedSizeInto(sqInt size, void *minAddress
DWORD lastError = GetLastError();
#if 0 /* Can't report this without making the system unusable... */
sqMessageBox(MB_OK | MB_ICONSTOP, TEXT("VM Error:"),
"Unable to VirtualAlloc committed memory at desired address (%" PRIuSQINT " bytes requested at %p, above %p), Error: %lu",
TEXT("Unable to VirtualAlloc committed memory at desired address (%") TEXT(PRIuSQINT) TEXT(" bytes requested at %p, above %p), Error: %lu"),
bytes, address, minAddress, lastError);
#else
if (fIsConsole)
Expand All @@ -175,7 +175,7 @@ sqAllocateMemorySegmentOfSizeAboveAllocatedSizeInto(sqInt size, void *minAddress
*/
if (alloc && !VirtualFree(alloc, SizeForRelease(bytes), MEM_RELEASE))
sqMessageBox(MB_OK | MB_ICONSTOP, TEXT("VM Warning:"),
"Unable to VirtualFree committed memory (%" PRIuSQINT " bytes requested), Error: %ul",
TEXT("Unable to VirtualFree committed memory (%") TEXT(PRIuSQINT) TEXT(" bytes requested), Error: %ul"),
bytes, GetLastError());
address += delta;
}
Expand All @@ -190,7 +190,7 @@ sqDeallocateMemorySegmentAtOfSize(void *addr, sqInt sz)
{
if (!VirtualFree(addr, SizeForRelease(sz), MEM_RELEASE))
sqMessageBox(MB_OK | MB_ICONSTOP, TEXT("VM Warning:"),
"Unable to VirtualFree committed memory (%" PRIuSQINT " bytes requested), Error: %ul",
TEXT("Unable to VirtualFree committed memory (%") TEXT(PRIuSQINT) TEXT(" bytes requested), Error: %ul"),
sz, GetLastError());
}

Expand Down Expand Up @@ -257,7 +257,7 @@ main()
return 0;
}
int __cdecl
sqMessageBox(DWORD dwFlags, const TCHAR *titleString, const char* fmt, ...)
sqMessageBox(DWORD dwFlags, const TCHAR *titleString, const TCHAR* fmt, ...)
{
va_list args;
int result;
Expand Down
16 changes: 7 additions & 9 deletions platforms/win32/vm/sqWin32Utils.c
Expand Up @@ -117,19 +117,17 @@ TCHAR *lstrrchr(TCHAR *source, TCHAR c)
/* printf() format string and arguments */
/****************************************************************************/
#ifndef sqMessageBox
int __cdecl sqMessageBox(DWORD dwFlags, const TCHAR *titleString, const char* fmt, ...)
{ TCHAR *ptr, *buf;
va_list args;
int __cdecl sqMessageBox(DWORD dwFlags, const TCHAR *titleString, const TCHAR* fmt, ...)
{ TCHAR *buf;
va_list args;
DWORD result;

ptr = toUnicodeNew(fmt);
buf = (TCHAR*) calloc(sizeof(TCHAR), 4096);
va_start(args, fmt);
wvsprintf(buf, ptr, args);
va_end(args);
va_start(args, fmt);
_vsntprintf(buf, 4096, fmt, args);
va_end(args);

result = MessageBox(stWindow,buf,titleString,dwFlags|MB_SETFOREGROUND);
free(ptr);
result = MessageBox(stWindow,buf,titleString,dwFlags|MB_SETFOREGROUND);
free(buf);
return result;
}
Expand Down

3 comments on commit 4f6f191

@krono
Copy link
Member

@krono krono commented on 4f6f191 Jan 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the '0' byte in line 127? do we need to calloc one byte more here?

@nicolas-cellier-aka-nice
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right!
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/vsnprintf-vsnprintf-vsnprintf-l-vsnwprintf-vsnwprintf-l?view=vs-2017
vsnprintf always writes the NULL terminator, and we can pass count=sizeof(buffer) as C99 mandates.
_vsnprintf does not, so we must set the buffer to zero, and pass count=sizeof(buffer)-1.
The Microsoft specific API is much too complex!

@krono
Copy link
Member

@krono krono commented on 4f6f191 Jan 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay for “standards”

Please sign in to comment.