On Windows we build VMPI_snprintf and VMPI_vsnprintf on top of _snprintf and _vsnprintf. However the latter functions do not conform to ISO C / common conventions, among other things they treat some formatting characters unusually and they do not have the same return values. (ISO C requires that the functions return the number of characters that would have been output had the buffer been large enough; the Windows versions provide the number of characters written into the buffer.) It is possible to build a simple conformant (or more conformant) implementation on top of the Windows functions, and we should do so.
Created attachment 434554 [details] [diff] [review] Selftests for VMPI_snprintf
Attachment #434554 - Flags: review?(edwsmith)
Created attachment 434556 [details] [diff] [review] Reasonably conformant implementation of VMPI_vsnprintf
Attachment #434556 - Flags: review?(edwsmith)
Priority: -- → P3
Whiteboard: Has patch
Target Milestone: Future → flash10.2
Edwin, review ping.
nit: an assert could protect fmtbuf from overrun, but we'll ever see such long format specifiiers nit: need to be un-tabify'd I reviewed all the code and can't find anything wrong. I didn't carefully read the VC++ docs to make sure all the format letters are handled (or handled correctly), and since the format specifiers are part of the VMPI api (the come from strings in our code) its more important they match the other platforms than having them match VC++ perfectly. Its good that this code delegates to the underlying impl.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
This code is actually chock-full of bugs: Buffer overruns because of several cases of *buffer++ = *format++ without checking the bound on buffer. Undiagnosed buffer overruns (fixing the above not enough to prevent crashes). The copystr case uses VMPI_strncpy but that's wrong because copystr is being jumped to from the %c case, which may actually output a NUL. Does not handle formatting of %s with a field width, and in general does not handle widths wider than the size of the output buffer, currently 600.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Additionally the selftests were too stringent for Solaris. Reverted the two changesets for this fix: tamarin-redux changeset: 4684:e295d183930f
Status: REOPENED → NEW
Priority: P3 → --
Target Milestone: flash10.2 → Future
You need to log in before you can comment on or make changes to this bug.