(PR#13283) R crashes on sprintf with bad format

From: r-devel-bounces at r-project.org
[mailto:r-devel-bounces at r-project.org] On Behalf Of Prof Brian Ripley
Sent: Friday, November 14, 2008 2:25 AM
To: Duncan Murdoch
Cc: R-bugs at r-project.org; ocheyett at bonddesk.com;
r-devel at stat.math.ethz.ch
Subject: Re: [Rd] (PR#13283) R crashes on sprintf with bad
format specification

As R's sprintf is a wrapper for the OS's sprintf, misuse does
run the risk of crashing from OS, and when it does the error
will come from the implementation of sprintf (which for R for
Windows is the Trio library).
One could argue that the OS service should not segfault on
incorrect input, but this one often does.

The place to add checks is in the R wrapper code (presumably
in the C component).  We do have checks there, but it is hard
to imagine just what errors users will make, and this one has
slipped through the checks.

The following change recognizes 'S' as a format type (but doesn't
do anything with it) and thus avoids the crash.

--- sprintf.c   (revision 46944)
+++ sprintf.c   (working copy)
@@ -103,7 +103,7 @@
               else {
                   /* recognise selected types from Table B-1 of K&R */
                   /* This is MBCS-OK, as we are in a format spec */
-                   chunk = strcspn(formatString + cur + 1,
"aAdisfeEgGxX%") + 2;
+                   chunk = strcspn(formatString + cur + 1,
"aAdisSfeEgGxX%") + 2;
                   if (cur + chunk > n)
                       error(_("unrecognised format at end of
string"));

The resulting error messages are not terribly direct, but nicer
than the crash

  > sprintf("A %S %S %S XYZ", 1, 1, 1)

  Error in sprintf("A %S %S %S XYZ", 1, 1, 1) :
    use format %f, %e, %g or %a for numeric objects

  > sprintf("A %S %S %S XYZ", "foo", "bar", "foo")

  Error in sprintf("A %S %S %S XYZ", "foo", "bar", "foo") :
    use format %s for character objects

One could add 'S' to some switch cases below there to have
it convert the corresponding argument to a wide character string
or say directly that %S is not allowed.

This piece of code finds the conversion type by looking for this
first of a bunch of known conversion characters after the %, which
doesn't find lots of illegal formats and causes some surprises.  E.g.,
because 'p' is not in that list we get:

  > sprintf("%p", 3)

  Error in sprintf("%p", 3) : unrecognised format at end of string

  > sprintf("%p is a pointer!", 3)

  [1] "0x3 is a pointer!"

  > sprintf("%p %o", 17L, 17L)

  [1] "0x4 o"

Here the issue is the use of %S, an unknown format for
numbers, followed by 'X'.  There is a check for a suitable
format, but it is not strict enough.  And it is non-trivial
to write a printf format parser just to check for user error
(and potentially it will slow down correct uses:
speed in sprintf is important in some applications).

So I am not sure this is something that should be R's
responsibility to
fix: maybe just add a warning to the help page?


On Thu, 13 Nov 2008, Duncan Murdoch wrote:

On 12/11/2008 8:30 PM, ocheyett at bonddesk.com wrote:

Full_Name: Oren Cheyette
Version: 2.7.2
OS: Win XP
Submission from: (NULL) (64.161.123.194)


Enter the following at the R command prompt:

sprintf("A %S %S %S XYZ", 1, 1, 1);

R is not C, and the empty command at the end of that line (after the
semicolon) is not relevant.

Note the erroneous capitalized %S instead of %s and the

numeric inputs

instead
of strings. With strings there's no crash - R reports bad format
specifications.

2.7.2 is obsolete, but I can confirm a crash on Windows

with a recent

R-devel.

The last few entries in the stack dump at the time of the

crash are shown

below; these make it look as though the problem is in the

Trio library, so it

may be hard to fix.

Duncan Murdoch


Rgui.exe caused an Access Violation at location 6c913fb3 in

module R.dll

Reading from location 00000001.

Registers:
eax=7fffffff ebx=00000000 ecx=00e17b21 edx=00000001

esi=00e1c83b edi=0000000a

eip=6c913fb3 esp=00e17944 ebp=00e17b3c iopl=0         nv up

ei pl nz na po nc

cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000 efl=00000206

Call stack:
6C913FB3  R.dll:6C913FB3  TrioFormatProcess  trio.c:2724

	...
	  while (length > 0)
	    {

	      size = TrioWriteWideStringCharacter(self,

*wstring++, flags,

length);
	      if (size == 0)
	break; /* while */
	...

6C916592  R.dll:6C916592  trio_vsprintf  trio.c:3771

	...
	    return status;

	  status = TrioFormatProcess(&data, format, parameters);

	  if (data.error != 0)
	    {
	...

6C911F62  R.dll:6C911F62  sprintf  compat.c:46

	...
	    va_end(ap);
	    return res;

	}

	...

6C889F1E  R.dll:6C889F1E  do_sprintf  sprintf.c:297

	...
	    sprintf(bit, fmtp, " NaN");
	else

	    sprintf(bit, fmtp, "NaN");

	    } else if (x == R_PosInf) {
	if (strcspn(fmtp, "+") < strlen(fmtp))
	...

--
Brian D. Ripley,                  ripley at stats.ox.ac.uk
Professor of Applied Statistics,  http://www.stats.ox.ac.uk/~ripley/
University of Oxford,             Tel:  +44 1865 272861 (self)
1 South Parks Road,                     +44 1865 272866 (PA)
Oxford OX1 3TG, UK                Fax:  +44 1865 272595