Hugh McMaster hugh.mcmaster@outlook.com wrote:
-static INT vmessagebox(HWND hwnd, INT buttons, INT titleId, INT resId, va_list ap) +static void load_string(unsigned int msg_id, WCHAR *buf, unsigned int count) { static const WCHAR errorW[] = {'E','r','r','o','r',0};
- static const WCHAR unknownW[] = {'U','n','k','n','o','w','n',' ','e','r','r','o','r',' ','s','t','r','i','n','g','!',0};
- if (!buf) return;
- if (!LoadStringW(hInst, msg_id, buf, count))
- {
WINE_FIXME("LoadString failed with %u\n", GetLastError());lstrcpyW(buf, errorW);- }
+}
Probably using a flavour of lstrcpy that checks the target buffer length would be more appropriate. Although simply returning an empty buffer and an error indicator to the caller could be slightly better. Under which conditions LoadString may fail here?
On Sunday, 29 January 2017 11:23 PM, Dmitry Timoshkov wrote:
Hugh McMaster wrote:
-static INT vmessagebox(HWND hwnd, INT buttons, INT titleId, INT resId, va_list ap) +static void load_string(unsigned int msg_id, WCHAR *buf, unsigned int count) { static const WCHAR errorW[] = {'E','r','r','o','r',0}; - static const WCHAR unknownW[] = {'U','n','k','n','o','w','n',' ','e','r','r','o','r',' ','s','t','r','i','n','g','!',0}; + if (!buf) return;
+ if (!LoadStringW(hInst, msg_id, buf, count)) + { + WINE_FIXME("LoadString failed with %u\n", GetLastError()); + lstrcpyW(buf, errorW); + } +}
Probably using a flavour of lstrcpy that checks the target buffer length would be more appropriate. Although simply returning an empty buffer and an error indicator to the caller could be slightly better. Under which conditions LoadString may fail here?
The buffer size must be at least six WCHARs to allow for errorW, so we could just do something like if (!buf || count < 6) return;
In this code, LoadString() could fail if the resource ID doesn't exist or if count is 0 or 1. But since we are controlling all of the code, none of this is likely.
For MessageBox captions, if we pass in NULL, the default string "Error" is loaded by user32. So, apart from a few instances, we wouldn't have to handle captions at all. But we would have to do something if LoadString() failed for the body text, otherwise the user would see a blank message box.
The original code was: if (!LoadStringW(hInst, titleId, title, COUNT_OF(title))) lstrcpyW(title, errorW); if (!LoadStringW(hInst, resId, errfmt, COUNT_OF(errfmt))) lstrcpyW(errfmt, unknownW);
Do you think it is worth replacing? My main reason for adding load_string() was to avoid duplicating the FIXME line and to avoid "Unknown error string".
-- Hugh
Hugh McMaster hugh.mcmaster@outlook.com wrote:
Probably using a flavour of lstrcpy that checks the target buffer length would be more appropriate. Although simply returning an empty buffer and an error indicator to the caller could be slightly better. Under which conditions LoadString may fail here?
The buffer size must be at least six WCHARs to allow for errorW, so we could just do something like if (!buf || count < 6) return;
In this code, LoadString() could fail if the resource ID doesn't exist or if count is 0 or 1. But since we are controlling all of the code, none of this is likely.
For MessageBox captions, if we pass in NULL, the default string "Error" is loaded by user32. So, apart from a few instances, we wouldn't have to handle captions at all. But we would have to do something if LoadString() failed for the body text, otherwise the user would see a blank message box.
The original code was: if (!LoadStringW(hInst, titleId, title, COUNT_OF(title))) lstrcpyW(title, errorW); if (!LoadStringW(hInst, resId, errfmt, COUNT_OF(errfmt))) lstrcpyW(errfmt, unknownW);
Do you think it is worth replacing? My main reason for adding load_string() was to avoid duplicating the FIXME line and to avoid "Unknown error string".
If LoadString should never fail under normal circumstances then perhaps simple assert(0) is enough, and there is no need to invent new code to handle errors that should never happen.
-
Dmitry Timoshkov -
Hugh McMaster