re: shlwapi: Resupply array sizes stripped by function interface
Andrew Talbot wrote:
A formal parameter declared as an array is treated as a pointer; any size specifier is ignored. So here, sizeof decimal_buffer, for example, would equate to the size of a pointer to WCHAR, not to that of an array of eight WCHARs.
Why are you doing this? - Dan
Dan Kegel wrote:
Andrew Talbot wrote:
A formal parameter declared as an array is treated as a pointer; any size specifier is ignored. So here, sizeof decimal_buffer, for example, would equate to the size of a pointer to WCHAR, not to that of an array of eight WCHARs.
Why are you doing this? - Dan
Hi Dan, Are you asking why I am doing lightweight static code checking, or why I am submitting this particular patch? -- Andy.
Andrew Talbot wrote:
A formal parameter declared as an array is treated as a pointer; any size specifier is ignored. So here, sizeof decimal_buffer, for example, would equate to the size of a pointer to WCHAR, not to that of an array of eight WCHARs.
Why are you doing this?
Are you asking why I am doing lightweight static code checking, or why I am submitting this particular patch?
The latter. I was clumsily trying to say "your changeset description isn't clear enough". If you had said "shlwapi: fix thinko in sizeof(array)" I might have woken up out of my stupor enough to understand the change. BTW the way you define the new size, as a magic constant, seems bad. Can you use 4 * sizeof(WCHAR), or whatever, instead of 8? And even then, the '4' seems almost as bad. - Dan
Dan Kegel wrote:
BTW the way you define the new size, as a magic constant, seems bad. Can you use 4 * sizeof(WCHAR), or whatever, instead of 8? And even then, the '4' seems almost as bad. - Dan
Yes, I did feel uneasy about using a magic constant, I must admit. Another way to handle it would be to call GetLocaleInfoW() twice for each buffer. The first time with its lpLCData parameter equal to NULL and its cchData parameter equal to zero, which should return the number of characters required for the buffer. The second time to then retrieve the required locale information. I had originally decided against this, because I felt that it might be over-engineering, but how does that sound to you? Also, if you would like to suggest a clearer changelog line, I would be very happy to use it. Thanks, -- Andy.
Andrew Talbot wrote:
Dan Kegel wrote:
BTW the way you define the new size, as a magic constant, seems bad. Can you use 4 * sizeof(WCHAR), or whatever, instead of 8? And even then, the '4' seems almost as bad. - Dan
Yes, I did feel uneasy about using a magic constant, I must admit. Another way to handle it would be to call GetLocaleInfoW() twice for each buffer. The first time with its lpLCData parameter equal to NULL and its cchData parameter equal to zero, which should return the number of characters required for the buffer. The second time to then retrieve the required locale information. I had originally decided against this, because I felt that it might be over-engineering, but how does that sound to you?
I've wrote that code and forgot that sizeof(parameter array) is the size of a pointer. The current code will actually work as in current wine there is no locale that has more than one character (plus the NUL terminator) for the decimal or thousand separator but it's better to fix it to use the whole buffer in case a locale with a strange separator is added. I've chosen the value 8 because I can't imagine a longer separator. I agree #defining a constant for it (and using it in FillNumberFmt, FormatInt, FormatDouble) would be good but writing it as 4*sizeof(WCHAR) isn't a good idea as the fourth parameter is the number of characters, not the number of bytes. Mikolaj Zalewski
Miko?aj Zalewski wrote:
Andrew Talbot wrote: I've wrote that code and forgot that sizeof(parameter array) is the size of a pointer. The current code will actually work as in current wine there is no locale that has more than one character (plus the NUL terminator) for the decimal or thousand separator but it's better to fix it to use the whole buffer in case a locale with a strange separator is added. I've chosen the value 8 because I can't imagine a longer separator. I agree #defining a constant for it (and using it in FillNumberFmt, FormatInt, FormatDouble) would be good but writing it as 4*sizeof(WCHAR) isn't a good idea as the fourth parameter is the number of characters, not the number of bytes.
Mikolaj Zalewski
Hi Mikolaj, I am about to submit another try, to see what people think. This time I'm using GetLocaleInfoW() twice for each buffer: once, to determine the size needed, and again, to actually get the locale info. -- Andy.
Andrew Talbot wrote:
I am about to submit another try, to see what people think. This time I'm using GetLocaleInfoW() twice for each buffer: once, to determine the size needed, and again, to actually get the locale info.
I'm afraid this is worse than it was - if a separator will happen to be longer than 7 characters plus NUL you will write part the end of the buffer (the buffers in FormatInt and FormatDouble are 8 characters long). If you really want to support strings of any length you should allocate the buffer dynamically. But IMHO that would be a wait of time for you and the CPU executing such code. I'd expect #defining a constant for the 8 as the buffer length should be enough. As for the changelog entry I'm not an expert but maybe "shlwapi: bugfix: sizeof(array passed as a parameter) can't be used to count it's elements" would be better? Mikolaj Zalewski
Miko?aj Zalewski wrote:
I'm afraid this is worse than it was - if a separator will happen to be longer than 7 characters plus NUL you will write part the end of the buffer (the buffers in FormatInt and FormatDouble are 8 characters long). If you really want to support strings of any length you should allocate the buffer dynamically. But IMHO that would be a wait of time for you and the CPU executing such code. I'd expect #defining a constant for the 8 as the buffer length should be enough. As for the changelog entry I'm not an expert but maybe "shlwapi: bugfix: sizeof(array passed as a parameter) can't be used to count it's elements" would be better?
Mikolaj Zalewski
I am now inclined to think - and in line with what you say - that it would be safer and more efficient to modify FillNumberFmt() to take two more arguments and pass the buffer sizes in as well, i.e., in effect, its declaration should be: static void FillNumberFmt(NUMBERFMTW *fmt, LPWSTR decimal_buffer, int decbuf_wlen, LPSTR thousand_buffer, int thoubuf_wlen); Does that sound better? -- Andy.
I am now inclined to think - and in line with what you say - that it would be safer and more efficient to modify FillNumberFmt() to take two more arguments and pass the buffer sizes in as well, i.e., in effect, its declaration should be:
static void FillNumberFmt(NUMBERFMTW *fmt, LPWSTR decimal_buffer, int decbuf_wlen, LPSTR thousand_buffer, int thoubuf_wlen);
Does that sound better?
-- Andy.
Yes, it's probably more elegant to send the buffer sizes explicitly as a parameter than to use a constant. Mikolaj Zalewski
participants (3)
-
Andrew Talbot -
Dan Kegel -
Mikołaj Zalewski