ISO C conformant snprintf and vsnprintf on Windows

NEW
Unassigned

Status

Tamarin
Virtual Machine
8 years ago
7 years ago

People

(Reporter: Lars T Hansen, Unassigned)

Tracking

unspecified
Future
All
Windows 7
Bug Flags:
flashplayer-qrb +

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 434554 [details] [diff] [review]
Selftests for VMPI_snprintf
Attachment #434554 - Flags: review?(edwsmith)
(Reporter)

Comment 2

8 years ago
Created attachment 434556 [details] [diff] [review]
Reasonably conformant implementation of VMPI_vsnprintf
Attachment #434556 - Flags: review?(edwsmith)
(Reporter)

Updated

8 years ago
Depends on: 554473
(Reporter)

Updated

8 years ago
Priority: -- → P3
Whiteboard: Has patch
Target Milestone: Future → flash10.2
(Reporter)

Comment 3

8 years ago
Edwin, review ping.

Updated

8 years ago
Attachment #434554 - Flags: review?(edwsmith) → review+

Comment 4

8 years ago
nit: an assert could protect fmtbuf[128] 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.

Updated

8 years ago
Attachment #434556 - Flags: review?(edwsmith) → review+
(Reporter)

Updated

8 years ago
Whiteboard: Has patch → has-patch
(Reporter)

Comment 5

8 years ago
tamarin-redux changeset:   4681:c962cfbbd26e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 6

8 years ago
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 → ---
(Reporter)

Comment 7

8 years ago
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 → --
Whiteboard: has-patch
Target Milestone: flash10.2 → Future
(Reporter)

Updated

8 years ago
Assignee: lhansen → nobody

Updated

7 years ago
Flags: flashplayer-qrb+
You need to log in before you can comment on or make changes to this bug.