710976 - Possible wrong error message in _cairo_win32_print_gdi_error

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Justin Dolske [:Dolske]]

From http://www.viva64.com/en/a/0078/

Example 9. Forming a wrong error message

cairo_status_t
_cairo_win32_print_gdi_error (const char *context)
{
  ...
  fwprintf(stderr, L"%s: %S", context, (wchar_t *)lpMsgBuf);
  ...
}

PVS-Studio diagnostic message: V576 Incorrect format. Consider checking the third actual argument of the 'fwprintf' function. The pointer to string of wchar_t type symbols is expected. cairo-win32-surface.c 129

Even if an error was successfully detected, it should be processed correctly. And since nobody tests error handlers either, we may find many interesting things there.

The _cairo_win32_print_gdi_error() function will print some trash. The fwprintf() function awaits a pointer to a unicode-string as the third argument, but instead it gets a string with the 'const char *' format.

Comment 1

[[e-mail]]

(In reply to Justin Dolske from comment #0)
>   fwprintf(stderr, L"%s: %S", context, (wchar_t *)lpMsgBuf);
In fact both format specifications are wrong; %s means "string of same wideness" while %S means "string of opposite wideness". Presumably this used to be an fprintf at some point when the format specifiers would have been correct.

Comment 2

[Jonathan Kew [:jfkthame]]

(In reply to [e-mail] from comment #1)
> (In reply to Justin Dolske from comment #0)
> >   fwprintf(stderr, L"%s: %S", context, (wchar_t *)lpMsgBuf);
> In fact both format specifications are wrong; %s means "string of same
> wideness" while %S means "string of opposite wideness". Presumably this used
> to be an fprintf at some point when the format specifiers would have been
> correct.

Not according to "man fwprintf" on OS X, at least....

<quote>
     S       Treated as s with the l (ell) modifier.

     s       The char * argument is expected to be a pointer to an array of character type (pointer to a string)
             containing a multibyte sequence.  Characters from the array are converted to wide characters and
             written up to (but not including) a terminating NUL character; if a precision is specified, no more
             than the number specified are written.  If a precision is given, no null character need be present;
             if the precision is not specified, or is greater than the size of the array, the array must contain
             a terminating NUL character.

             If the l (ell) modifier is used, the wchar_t * argument is expected to be a pointer to an array of
             wide characters (pointer to a wide string).  Each wide character in the string is written.  Wide
             characters from the array are written up to (but not including) a terminating wide NUL character;
             if a precision is specified, no more than the number specified are written (including shift
             sequences).  If a precision is given, no null character need be present; if the precision is not
             specified, or is greater than the number of characters in the string, the array must contain a ter-
             minating wide NUL character.
</quote>

So this claims that %s is always char* and %S is always wchar_t*.

/me checks the MSDN docs.....

Hmmm, so according to Microsoft, things are indeed as you claim. IOW, unless someone's doc is wrong, the MS and BSD versions of the ?wprintf functions are incompatible. (Sigh.) The same applies to the %c and %C specifiers.

As this occurs in a win32-only source file, it had better follow Microsoft's rules.

And one more thing: it looks like we patched this previously; see gfx/cairo/fix-cairo-win32-print-gdi-error.diff. I'm guessing the fix got lost during a cairo update. :(  Cc-ing Jeff, who usually handles those AFAIK.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jonathan Kew (:jfkthame) from comment #2)

And one more thing: it looks like we patched this previously; see
gfx/cairo/fix-cairo-win32-print-gdi-error.diff. I'm guessing the fix got
lost during a cairo update. :( Cc-ing Jeff, who usually handles those AFAIK.

Looks like you didn't forget to re-apply it when you fixed bug 739096 :-)