R devel - Apr 2009 - incorrect output and segfaults from sprintf with %*d (PR#13667)

Full_Name: Wacek Kusnierczyk
Version: 2.10.0 r48365
OS: Ubuntu 8.04 Linux 32bit
Submission from: (NULL) (129.241.110.141)


sprintf has a documented limit on strings included in the output using the
format '%s'.  It appears that there is a limit on the length of strings
included
with, e.g., the format '%d' beyond which surprising things happen
(output
modified for conciseness):

   gregexpr('1', sprintf('%9000d', 1))
   # [1] 9000 9801

   gregexpr('1', sprintf('%9000d', 1))
   # [1]  9000  9801 10602

   gregexpr('1', sprintf('%9000d', 1))
   # [1]  9000  9801 10602 11403

   gregexpr('1', sprintf('%9000d', 1))
   # [1]  9000  9801 10602 11403 12204

   ...

Note that not only more than one '1' is included in the output, but also
that
the same functional expression (no side effects used beyond the interface) gives
different results on each execution.  Analogous behaviour can be observed with
'%nd' where n > 8200.

The actual output above is consistent across separate sessions.

With sufficiently large field width values, R segfaults:

   sprintf('%*d', 10^5, 1)
   # *** caught segfault ***
   # address 0xbfcfc000, cause 'memory not mapped'
   # Segmentation fault


   sessionInfo()
   # R version 2.10.0 Under development (unstable) (2009-04-20 r48365) 
   # i686-pc-linux-gnu

>>>>> "vQ" == Wacek Kusnierczyk <waku at
idi.ntnu.no>
>>>>>     on Tue, 21 Apr 2009 13:05:11 +0200 (CEST) writes:
    vQ> Full_Name: Wacek Kusnierczyk
    vQ> Version: 2.10.0 r48365
    vQ> OS: Ubuntu 8.04 Linux 32bit
    vQ> Submission from: (NULL) (129.241.110.141)


    vQ> sprintf has a documented limit on strings included in the output
using the
    vQ> format '%s'.  It appears that there is a limit on the length
of strings included
    vQ> with, e.g., the format '%d' beyond which surprising things
happen (output
    vQ> modified for conciseness):

    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1] 9000 9801

    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602

    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602 11403

    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602 11403 12204

    vQ> ...

    vQ> Note that not only more than one '1' is included in the
output, but also that
    vQ> the same functional expression (no side effects used beyond the
interface) gives
    vQ> different results on each execution.  Analogous behaviour can be
observed with
    vQ> '%nd' where n > 8200.

    vQ> The actual output above is consistent across separate sessions.

    vQ> With sufficiently large field width values, R segfaults:

    vQ> sprintf('%*d', 10^5, 1)
    vQ> # *** caught segfault ***
    vQ> # address 0xbfcfc000, cause 'memory not mapped'
    vQ> # Segmentation fault


Thank you, Wacek.
That's all ``interesting''  ... unfortunately, 

my version of  'man 3 sprintf' contains
>> BUGS
>>        Because sprintf() and vsprintf() assume an arbitrarily
>>        long string, callers must be careful not to overflow the
>>        actual space; this is often impossible to assure. Note
>>        that the length of the strings produced is
>>        locale-dependent and difficult to predict.  Use
>>        snprintf() and vsnprintf() instead (or asprintf() and
vasprintf).
(note the "impossible" part above)       
and we haven't used  snprintf() yet, probably because it
requires the  C99 C standard, and AFAIK, we have only relatively
recently started to more or less rely on C99 in the R sources.
       
More precisely, I see that some windows-only code relies on
snprintf() being available  whereas in at least on non-Windows
section, I read   /* we cannot assume snprintf here */

Now such platform dependency issues and corresponding configure
settings I do typically leave to other R-corers with a much
wider overview about platforms and their compilers and C libraries.
       

BTW,  
1) sprintf("%n %g", 1,1)   also seg.faults

2) Did you have a true use case where  the  8192  limit was an
   undesirable limit?

Martin       

    vQ> sessionInfo()
    vQ> # R version 2.10.0 Under development (unstable) (2009-04-20 r48365) 
    vQ> # i686-pc-linux-gnu

>>>>> "vQ" == Wacek Kusnierczyk
<Waclaw.Marcin.Kusnierczyk at idi.ntnu.no>
>>>>>     on Thu, 23 Apr 2009 11:49:54 +0200 writes:
    vQ> maechler at stat.math.ethz.ch wrote:
    >> 
    vQ> sprintf has a documented limit on strings included in the output
using the
    vQ> format '%s'.  It appears that there is a limit on the length
of strings included
    vQ> with, e.g., the format '%d' beyond which surprising things
happen (output
    vQ> modified for conciseness):
    >> 

    vQ> ... and this limit is *not* documented.

well, it is basically (+ a few bytes ?)
the same  8192  limit that *is* documented.

    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1] 9000 9801
    >> 
    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602
    >> 
    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602 11403
    >> 
    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602 11403 12204
    >> 
    vQ> ...
    >> 
    vQ> Note that not only more than one '1' is included in the
output, but also that
    vQ> the same functional expression (no side effects used beyond the
interface) gives
    vQ> different results on each execution.  Analogous behaviour can be
observed with
    vQ> '%nd' where n > 8200.
    >> 
    vQ> The actual output above is consistent across separate sessions.
    >> 
    vQ> With sufficiently large field width values, R segfaults:
    >> 
    vQ> sprintf('%*d', 10^5, 1)
    vQ> # *** caught segfault ***
    vQ> # address 0xbfcfc000, cause 'memory not mapped'
    vQ> # Segmentation fault
    >> 
    >> 
    >> Thank you, Wacek.
    >> That's all ``interesting''  ... unfortunately, 
    >> 
    >> my version of  'man 3 sprintf' contains
    >> 
    >> 
    >>>> BUGS
    >>>> Because sprintf() and vsprintf() assume an arbitrarily
    >>>> long string, callers must be careful not to overflow the
    >>>> actual space; this is often impossible to assure. Note
    >>>> that the length of the strings produced is
    >>>> locale-dependent and difficult to predict.  Use
    >>>> snprintf() and vsnprintf() instead (or asprintf() and
vasprintf).

    vQ> yes, but this is c documentation, not r documentation.

Of course! ...  and I  *do*  apply it to R's C code [sprintf.c]
and hence am even concurring with you .. 

    vQ> it's applicable
    vQ> to a degree, since ?sprintf does say that sprintf is "a wrapper
for the
    vQ> C function 'sprintf'".  however, in c you use a buffer
and you usually
    vQ> have control over it's capacity, while in r this is a hidden
    vQ> implementational detail, which should not be visible to the user, or
    vQ> should cause an attempt to overflow the buffer to fail more
gracefully
    vQ> than with a segfault.

    vQ> in r, sprintf('%9000d', 1) will produce a confused output
with a count
    vQ> of 1's variable (!) across runs (while sprintf('%*d',
9000, 1) seems to
    vQ> do fine):

    vQ> gregexpr('1', sprintf('%*d', 9000, 1))
    vQ> # [1] 9000

    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1] 9000 9801 ..., variable across executions

    vQ> on one execution in a series i actually got this:

    vQ> Warning message:
    vQ> In gregexpr("1", sprintf("%9000d", 1)) :
    vQ> input string 1 is invalid in this locale

    vQ> while the very next execution, still in the same session, gave

    vQ> # [1]  9000  9801 10602

    vQ> with sprintf('%*d', 10000, 1) i got segfaults on some
executions but
    vQ> correct output on others, while sprintf('%10000d', 1) is
confused again.



    >> (note the "impossible" part above)       
    >> 

    vQ> yes, but it does also say "must be careful", and it seems
that someone
    vQ> has not been careful enough.

    >> and we haven't used  snprintf() yet, probably because it
    >> requires the  C99 C standard, and AFAIK, we have only relatively
    >> recently started to more or less rely on C99 in the R sources.
    >> 

    vQ> while snprintf would help avoid buffer overflow, it may not be a
    vQ> solution to the issue of confused output.

I think it would / will.  We would be able to give warnings and
errors, by checking the  snprintf()  return codes.


    >> More precisely, I see that some windows-only code relies on
    >> snprintf() being available  whereas in at least on non-Windows
    >> section, I read   /* we cannot assume snprintf here */
    >> 
    >> Now such platform dependency issues and corresponding configure
    >> settings I do typically leave to other R-corers with a much
    >> wider overview about platforms and their compilers and C libraries.
    >> 

    vQ> it looks like src/main/sprintf.c is just buggy, and it's
plausible that
    vQ> the bug could be repaired in a platform-independent manner.

definitely.
In the mean time, I've actually found that what I first said on
the usability of snprintf() in R's code base was only partly correct.
There are other parts of R code where we use  snprintf() for all
platforms, hence we rely on its presence (and correct
implementation!) and so we can and I think should use it in
place of sprintf() in quite a few places inside R's sprintf.c


    >> BTW,  
    >> 1) sprintf("%n %g", 1,1)   also seg.faults
    >> 

    vQ> as do

    vQ> sprintf('%n%g', 1, 1)
    vQ> sprintf('%n%')

    vQ> etc., while

    vQ> sprintf('%q%g', 1, 1)
    vQ> sprintf('%q%')
  
    vQ> work just fine.  strange, because per ?sprintf 'n' is not
recognized as
    vQ> a format specifier, so the output from the first two above should be
as
    vQ> from the last two above, respectively.  (and likewise in the %S case,
    vQ> discussed and bug-reported earlier.)

I have now fixed these bugs at least;
the more subtle  "%<too_large_n>d" ones are different, and
as I said, I'm convinced that a nice & clean fix for those will
start using snprintf().

    >> 2) Did you have a true use case where  the  8192  limit was an
    >> undesirable limit?

    vQ> how does it matter?  

well, we could increase it, if it did matter.
      {{ you *could* have been more polite here, no?
      	 it *was* after all a serious question that I asked! }}

    vQ>  if you set a limit, be sure to consistently enforce
    vQ> it and warn the user on attempts to exceed it.  or write clearly in
the
    vQ> docs that such attempts will cause the output to be silently
truncated.

Sure, I'm not at all disagreeing on that, and if you read this into my
posting, you misunderstand.

Martin

    vQ> examples such as

    vQ> sprintf('%9000d', 1)

    vQ> do not contribute to the reliability of r, and neither to the
user's
    vQ> confidence in it.

    vQ> vQ

Martin Maechler wrote:
>>>>>> "vQ" == Wacek Kusnierczyk
<Waclaw.Marcin.Kusnierczyk at idi.ntnu.no>
>>>>>>     on Thu, 23 Apr 2009 11:49:54 +0200 writes:
>>>>>>             
>
>     vQ> maechler at stat.math.ethz.ch wrote:
>     >> 
>     vQ> sprintf has a documented limit on strings included in the output
using the
>     vQ> format '%s'.  It appears that there is a limit on the
length of strings included
>     vQ> with, e.g., the format '%d' beyond which surprising
things happen (output
>     vQ> modified for conciseness):
>     >> 
>
>     vQ> ... and this limit is *not* documented.
>
> well, it is basically (+ a few bytes ?)
> the same  8192  limit that *is* documented.
>   
martin, ?sprintf says:

" There is a limit of 8192 bytes on elements of 'fmt' and also on
     strings included by a '%s' conversion specification."

for me, it's clear that the *elements of fmt* cannot be longer than 8192
bytes, and that each single bit included in the output in place of a %s
cannot be longer than 8192.  nowhere does it say that strings included
in the output in place of a %d, for example, cannot be longer than
8192.  the fact that %s is particularized makes me infer that there is
something specific to %s that does not apply to %d, for example,
otherwise the help would have been formulated differently.  (though
given how r help pages are written, nothing seems unlikely.)

and in fact, the limit does not seem to apply in an obvious way in cases
such as sprintf('%*d', 10000, 1), where the output is correct.  at the
very least, the documentation leaves the user ignorant as to what will
happen if the limit is exceeded.
>     >> my version of  'man 3 sprintf' contains
>     >> 
>     >> 
>     >>>> BUGS
>     >>>> Because sprintf() and vsprintf() assume an arbitrarily
>     >>>> long string, callers must be careful not to overflow
the
>     >>>> actual space; this is often impossible to assure. Note
>     >>>> that the length of the strings produced is
>     >>>> locale-dependent and difficult to predict.  Use
>     >>>> snprintf() and vsnprintf() instead (or asprintf() and
vasprintf).
>
>     vQ> yes, but this is c documentation, not r documentation.
>
> Of course! ...  and I  *do*  apply it to R's C code [sprintf.c]
> and hence am even concurring with you .. 
>
>
>     vQ> while snprintf would help avoid buffer overflow, it may not be a
>     vQ> solution to the issue of confused output.
>
> I think it would / will.  We would be able to give warnings and
> errors, by checking the  snprintf()  return codes.
>   
maybe, i can't judge without carefully examining the code for sprintf.c
(which i am rather unwilling to do, having had a look at a sample).
>
>     >> More precisely, I see that some windows-only code relies on
>     >> snprintf() being available  whereas in at least on non-Windows
>     >> section, I read   /* we cannot assume snprintf here */
>     >> 
>     >> Now such platform dependency issues and corresponding
configure
>     >> settings I do typically leave to other R-corers with a much
>     >> wider overview about platforms and their compilers and C
libraries.
>     >> 
>
>     vQ> it looks like src/main/sprintf.c is just buggy, and it's
plausible that
>     vQ> the bug could be repaired in a platform-independent manner.
>
> definitely.
> In the mean time, I've actually found that what I first said on
> the usability of snprintf() in R's code base was only partly correct.
> There are other parts of R code where we use  snprintf() for all
> platforms, hence we rely on its presence (and correct
> implementation!) and so we can and I think should use it in
> place of sprintf() in quite a few places inside R's sprintf.c
>
>   
would be interesting to see how this improves sprintf.
>     >> BTW,  
>     >> 1) sprintf("%n %g", 1,1)   also seg.faults
>     >> 
>
>     vQ> as do
>
>     vQ> sprintf('%n%g', 1, 1)
>     vQ> sprintf('%n%')
>
>     vQ> etc., while
>
>     vQ> sprintf('%q%g', 1, 1)
>     vQ> sprintf('%q%')
>   
>     vQ> work just fine.  strange, because per ?sprintf 'n' is
not recognized as
>     vQ> a format specifier, so the output from the first two above
should be as
>     vQ> from the last two above, respectively.  (and likewise in the %S
case,
>     vQ> discussed and bug-reported earlier.)
>
> I have now fixed these bugs at least;
>   
great, i'm going to torture the fix soon ;)
> the more subtle  "%<too_large_n>d" ones are different, and
> as I said, I'm convinced that a nice & clean fix for those will
> start using snprintf().
>
>     >> 2) Did you have a true use case where  the  8192  limit was an
>     >> undesirable limit?
>
>     vQ> how does it matter?  
>
> well, we could increase it, if it did matter.
>       {{ you *could* have been more polite here, no?
>   
i don't see how i could be more polite here, i had absolutely no
intention to be impolite and didn't think i were. 

i gave a serious answer by means of a serious question.  increasing an
arbitrary, poorly documented limit of obscure effect is hardly any
solution.  suggesting that a bug is not a bug because some limit is not
likely to be exceeded in practice is not a particularly good idea.

>       	 it *was* after all a serious question that I asked! }}
>
>     vQ>  if you set a limit, be sure to consistently enforce
>     vQ> it and warn the user on attempts to exceed it.  or write clearly
in the
>     vQ> docs that such attempts will cause the output to be silently
truncated.
>
> Sure, I'm not at all disagreeing on that, and if you read this into my
> posting, you misunderstand.
>   
no, i didn't read that into your posting, i'm just referring to the
state of the 'art' in r.

cheers,
vQ

>>>>> "vQ" == Wacek Kusnierczyk
<Waclaw.Marcin.Kusnierczyk at idi.ntnu.no>
>>>>>     on Thu, 23 Apr 2009 15:00:29 +0200 writes:
    vQ> Martin Maechler wrote:
    >>>>>>> "vQ" == Wacek Kusnierczyk
<Waclaw.Marcin.Kusnierczyk at idi.ntnu.no>
    >>>>>>> on Thu, 23 Apr 2009 11:49:54 +0200 writes:


[......................]
[......................]

    >> >> BTW,  
    >> >> 1) sprintf("%n %g", 1,1)   also seg.faults
    >> >> 
    >> 
    vQ> as do
    >> 
    vQ> sprintf('%n%g', 1, 1)
    vQ> sprintf('%n%')
    >> 
    vQ> etc., while
    >> 
    vQ> sprintf('%q%g', 1, 1)
    vQ> sprintf('%q%')
    >> 
    vQ> work just fine.  strange, because per ?sprintf 'n' is not
recognized as
    vQ> a format specifier, so the output from the first two above should be
as
    vQ> from the last two above, respectively.  (and likewise in the %S case,
    vQ> discussed and bug-reported earlier.)
    >> 
    >> I have now fixed these bugs at least;
    >> 

    vQ> great, i'm going to torture the fix soon ;)

there will be another one, still today, fixing the

      sprintf("%s", tryCatch(stop(), error=identity))

bug {which actually *is* a subtle, too}

    >> the more subtle  "%<too_large_n>d" ones are
different, and
    >> as I said, I'm convinced that a nice & clean fix for those
will
    >> start using snprintf().
    >> 
    >> >> 2) Did you have a true use case where  the  8192  limit
was an
    >> >> undesirable limit?
    >> 
    vQ> how does it matter?  
    >> 
    >> well, we could increase it, if it did matter.
    >> {{ you *could* have been more polite here, no?
    >> 

    vQ> i don't see how i could be more polite here, i had absolutely no
    vQ> intention to be impolite and didn't think i were. 

    vQ> i gave a serious answer by means of a serious question.  increasing
an
    vQ> arbitrary, poorly documented limit of obscure effect is hardly any
    vQ> solution.  suggesting that a bug is not a bug because some limit is
not
    vQ> likely to be exceeded in practice is not a particularly good idea.

But that's exactly what I did  NOT  suggest!!

It was a serious question, *related* to your bug report, but
*NOT* really on the bug report proper.
[It *was* under the "heading" of 'BTW' which I assumed you
knew
 to interpret]
I was seriously asking, if BTW, the limit which is there was
possibly to be increased or not...


    >> it *was* after all a serious question that I asked! }}
    >> 
    vQ> if you set a limit, be sure to consistently enforce
    vQ> it and warn the user on attempts to exceed it.  or write clearly in
the
    vQ> docs that such attempts will cause the output to be silently
truncated.
    >> 
    >> Sure, I'm not at all disagreeing on that, and if you read this
into my
    >> posting, you misunderstand.
    >> 

    vQ> no, i didn't read that into your posting, i'm just referring
to the
    vQ> state of the 'art' in r.

[not so funny...  yet another "very polite" assumption.]

    vQ> cheers,
    vQ> vQ

>>>>> "MM" == Martin Maechler <maechler at
stat.math.ethz.ch>
>>>>>     on Thu, 23 Apr 2009 12:40:22 +0200 (CEST) writes:
>>>>> "vQ" == Wacek Kusnierczyk
<Waclaw.Marcin.Kusnierczyk at idi.ntnu.no>
>>>>>     on Thu, 23 Apr 2009 11:49:54 +0200 writes:
    vQ> maechler at stat.math.ethz.ch wrote:
    >>> 
    vQ> sprintf has a documented limit on strings included in the output
using the
    vQ> format '%s'.  It appears that there is a limit on the length
of strings included
    vQ> with, e.g., the format '%d' beyond which surprising things
happen (output
    vQ> modified for conciseness):
    >>> 

    vQ> ... and this limit is *not* documented.

    MM> well, it is basically (+ a few bytes ?)
    MM> the same  8192  limit that *is* documented.

indeed, I was right with that..

    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1] 9000 9801
    >>> 
    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602
    >>> 
    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602 11403
    >>> 
    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1]  9000  9801 10602 11403 12204
    >>> 
    vQ> ...
    >>> 

    vQ> Note that not only more than one '1' is included in the
    vQ> output, but also that the same functional expression (no
    vQ> side effects used beyond the interface) gives different
    vQ> results on each execution.  Analogous behaviour can be
    vQ> observed with '%nd' where n > 8200.

    vQ> The actual output above is consistent across separate sessions.
    >>> 
    vQ> With sufficiently large field width values, R segfaults:
    >>> 
    vQ> sprintf('%*d', 10^5, 1)
    vQ> # *** caught segfault ***
    vQ> # address 0xbfcfc000, cause 'memory not mapped'
    vQ> # Segmentation fault
    >>> 
    >>> 
    >>> Thank you, Wacek.
    >>> That's all ``interesting''  ... unfortunately, 
    >>> 
    >>> my version of  'man 3 sprintf' contains
    >>> 
    >>> 
    >>>>> BUGS
    >>>>> Because sprintf() and vsprintf() assume an arbitrarily
    >>>>> long string, callers must be careful not to overflow
the
    >>>>> actual space; this is often impossible to assure. Note
    >>>>> that the length of the strings produced is
    >>>>> locale-dependent and difficult to predict.  Use
    >>>>> snprintf() and vsnprintf() instead (or asprintf() and
vasprintf).

    vQ> yes, but this is c documentation, not r documentation.

    MM> Of course! ...  and I  *do*  apply it to R's C code [sprintf.c]
    MM> and hence am even concurring with you .. 

    vQ> it's applicable
    vQ> to a degree, since ?sprintf does say that sprintf is "a wrapper
for the
    vQ> C function 'sprintf'".  however, in c you use a buffer
and you usually
    vQ> have control over it's capacity, while in r this is a hidden
    vQ> implementational detail, which should not be visible to the user, or
    vQ> should cause an attempt to overflow the buffer to fail more
gracefully
    vQ> than with a segfault.

    vQ> in r, sprintf('%9000d', 1) will produce a confused output
with a count
    vQ> of 1's variable (!) across runs (while sprintf('%*d',
9000, 1) seems to
    vQ> do fine):

    vQ> gregexpr('1', sprintf('%*d', 9000, 1))
    vQ> # [1] 9000

    vQ> gregexpr('1', sprintf('%9000d', 1))
    vQ> # [1] 9000 9801 ..., variable across executions

    vQ> on one execution in a series i actually got this:

    vQ> Warning message:
    vQ> In gregexpr("1", sprintf("%9000d", 1)) :
    vQ> input string 1 is invalid in this locale

    vQ> while the very next execution, still in the same session, gave

    vQ> # [1]  9000  9801 10602

    vQ> with sprintf('%*d', 10000, 1) i got segfaults on some
executions but
    vQ> correct output on others, while sprintf('%10000d', 1) is
confused again.


    >>> (note the "impossible" part above)       
    >>> 

    vQ> yes, but it does also say "must be careful", and it seems
that someone
    vQ> has not been careful enough.

    >>> and we haven't used  snprintf() yet, probably because it
    >>> requires the  C99 C standard, and AFAIK, we have only
relatively
    >>> recently started to more or less rely on C99 in the R sources.
    >>> 

    vQ> while snprintf would help avoid buffer overflow, it may not be a
    vQ> solution to the issue of confused output.

    MM> I think it would / will.  We would be able to give warnings and
    MM> errors, by checking the  snprintf()  return codes.

My current working code gives an error for all the above
examples, e.g.,

 > sprintf('%9999d', 1)
 Error in sprintf("%9999d", 1) : 
   required resulting string length 9999 is > maximal 8191

it passes  'make check-devel' and I am inclined to commit that
code to R-devel (e.g. tomorrow). 

Yes, the documentation will also have to be amended, but apart
from that, would people see a big problem with the "8192" limit
which now is suddenly of greater importance
{{as I said all along;  hence my question to Wacek (and the
  R-develers)  if anybody found that limit too low}}

Of course, one could realloc()ate longer strings when needed
inside R's  sprintf() code,  but I reluctant to do that (render
the code yet more complicated ...) if nobody sees a need.

Martin

    >>> More precisely, I see that some windows-only code relies on
    >>> snprintf() being available  whereas in at least on non-Windows
    >>> section, I read   /* we cannot assume snprintf here */
    >>> 
    >>> Now such platform dependency issues and corresponding configure
    >>> settings I do typically leave to other R-corers with a much
    >>> wider overview about platforms and their compilers and C
libraries.
    >>> 

    vQ> it looks like src/main/sprintf.c is just buggy, and it's
plausible that
    vQ> the bug could be repaired in a platform-independent manner.

    MM> definitely.
    MM> In the mean time, I've actually found that what I first said on
    MM> the usability of snprintf() in R's code base was only partly
correct.
    MM> There are other parts of R code where we use  snprintf() for all
    MM> platforms, hence we rely on its presence (and correct
    MM> implementation!) and so we can and I think should use it in
    MM> place of sprintf() in quite a few places inside R's sprintf.c


    >>> BTW,  
    >>> 1) sprintf("%n %g", 1,1)   also seg.faults
    >>> 

    vQ> as do

    vQ> sprintf('%n%g', 1, 1)
    vQ> sprintf('%n%')

    vQ> etc., while

    vQ> sprintf('%q%g', 1, 1)
    vQ> sprintf('%q%')
  
    vQ> work just fine.  strange, because per ?sprintf 'n' is not
recognized as
    vQ> a format specifier, so the output from the first two above should be
as
    vQ> from the last two above, respectively.  (and likewise in the %S case,
    vQ> discussed and bug-reported earlier.)

    MM> I have now fixed these bugs at least;
    MM> the more subtle  "%<too_large_n>d" ones are
different, and
    MM> as I said, I'm convinced that a nice & clean fix for those
will
    MM> start using snprintf().

    >>> 2) Did you have a true use case where  the  8192  limit was an
    >>> undesirable limit?

    vQ> how does it matter?  

    MM> well, we could increase it, if it did matter.
    MM> {{ you *could* have been more polite here, no?
    MM> it *was* after all a serious question that I asked! }}

    vQ> if you set a limit, be sure to consistently enforce
    vQ> it and warn the user on attempts to exceed it.  or write clearly in
the
    vQ> docs that such attempts will cause the output to be silently
truncated.

    MM> Sure, I'm not at all disagreeing on that, and if you read this
into my
    MM> posting, you misunderstand.

    MM> Martin

    [..........................]

Martin Maechler wrote:
>
>>>     MM> well, it is basically (+ a few bytes ?)
>>>     MM> the same  8192  limit that *is* documented.
>>>
>>> indeed, I was right with that..
>>>
>>>       
>> hmm, i'd guess this limit is valid for all strings included in the
>> output with any format?  not just %s (and, as it appears,
undocumentedly
>> %d)?
>>     
>
> yes.
>   
so it's perhaps easiest to change

" There is a limit of 8192 bytes on elements of 'fmt' and also on
     strings included by a '%s' conversion specification."

to sth like

" There is a limit of 8192 bytes on elements of 'fmt' and also on
     strings included in the output by any conversion specification."

it's in fact so easy that even i should be able to do it.

[1 minute later...]  i see you've fixed this one, too.

>> btw. (i do know what that means ;)), after your recent fix:
>>
>>    sprintf('%q%s', 1)
>>    # Error in sprintf("%q%s", 1) :
>>    #  use format %f, %e, %g or %a for numeric objects
>>
>>    sprintf('%s', 1)
>>    # [1] "1"
>>
>> you may want to add '%s' (and '%x', and ...) to the
error message.  or
>> perhaps make it say sth like 'invalid format: ...'.  the
problem is not
>> that %q is not applicable to numeric, but that it is not a valid format
>> at all.
>>     
>
> yes.  As a matter of fact,  "%q%s" is dealt with as *one* format
> chunk, since "%q" is
> not a valid format.
> The code I have just committed now gives a longer erro message, that
> should be more helpful.
>   
yes, but

    sptinf('%q%s', 1)


still suggests that one uses %{f,e,g,a} for numerics, while %s is pretty
much valid, too.  you see, in c sprintf(buffer, "%s", 1) is destined
to
cause a segfault, but in r it works -- so the error message is slightly
misleading, as it suggests %s is *not* valid for numerics.
> Thank you for the suggestion!
>   
yo welcum
>   
>> there's also an issue with the additional arguments supplied after
the
>> format:  any superfluous arguments are ignored (this is not documented,
>> as far as i can see),
>>     
>
> I think we should signal an error if there are too many arguments.
>   
agree.  but it might be more complex than it appears:

    sprintf('%3$s', 1, 2, 3)

should *not* complain about too many args, despite just one conversion
spec in the format.  interestingly,

    sprintf('%3$s', , , 3)
    # error: argument is missing, with no default

> Could anyone think of a case where the current behavior is desirable ?
>   
well, one scenario might be that one wants to print a collection of
items with an arbitrary format, supplied by the users, e.g.

    foo = function(fmt) {
       a = ...
       b = ...
       ...
       s = sprintf(fmt, a, b, ...)
       ... }

without having to examine the format to establish which values are
needed.  in the current state, sprintf would use those it would need to
use with a particular format, with no undesirable complaints.

>   
>> but they *are* evaluated nevertheless, e.g.:
>>
>>    sprintf('%d', 0, {print(1)})
>>    # "1"
>>    # [1] "0"
>>
>> it might be a good idea to document this behaviour.
>>     
>
> actually I think it should be changed to be more strict (see above).
>   
strict in which sense?  enforce a constraint on the number of arguments
to that needed by a specific format?  or do you mean evaluation of only
those arguments that are needed in a format?  or both?

what about:

    sprintf('%2$s', {print(1)}, 2)
    # too many arguments?
    # should 1 be printed?

    sprintf('%2$s', , 2)
    # too few arguments?
    # missing value?  (yes, sprintf is .Internal, but...)

> Thank you for the constructive feedback!
>   
not much to thank for...  certainly, it's the first time my feedback is
called 'constructive'.  i'm making progress, am i not?

btw., thank you for the fixes.  i appreciate your efforts a lot.

best,
vQ

>>>>> "vQ" == Waclaw Marcin Kusnierczyk
<Waclaw.Marcin.Kusnierczyk at idi.ntnu.no>
>>>>>     on Sat, 25 Apr 2009 19:40:27 +0200 (CEST) writes:
    vQ> Martin Maechler wrote:
    >> 
    MM> well, it is basically (+ a few bytes ?)  the same 8192
    MM> limit that *is* documented.
    >>>> 
    >>>> indeed, I was right with that..
    >>>> 
    >>>> 
    >>> hmm, i'd guess this limit is valid for all strings
    >>> included in the output with any format?  not just %s
    >>> (and, as it appears, undocumentedly %d)?
    >>> 
    >> 


    >>> btw. (i do know what that means ;)), after your recent
    >>> fix:
    >>> 
    >>> sprintf('%q%s', 1) # Error in sprintf("%q%s",
1) : # use
    >>> format %f, %e, %g or %a for numeric objects
    >>> 
    >>> sprintf('%s', 1) # [1] "1"
    >>> 
    >>> you may want to add '%s' (and '%x', and ...) to
the
    >>> error message.  or perhaps make it say sth like 'invalid
    >>> format: ...'.  the problem is not that %q is not
    >>> applicable to numeric, but that it is not a valid format
    >>> at all.
    >>> 
    >> 
> yes.  As a matter of fact, "%q%s" is dealt with as *one*
    >> format chunk, since "%q" is not a valid format.  The code
    >> I have just committed now gives a longer erro message,
    >> that should be more helpful.
    >> 

    vQ> yes, but

    vQ>     sptinf('%q%s', 1)


    vQ> still suggests that one uses %{f,e,g,a} for numerics,
    vQ> while %s is pretty much valid, too.  you see, in c
    vQ> sprintf(buffer, "%s", 1) is destined to cause a
    vQ> segfault, but in r it works -- so the error message is
    vQ> slightly misleading, as it suggests %s is *not* valid
    vQ> for numerics.

yes, but only the error message somewhat suggests that;
at the moment, I'd like to keep it, since really the user
*should* think of using the number formats, rather than %s
{which just calls  as.character(.)}  for numeric arguments


    >> Thank you for the suggestion!
    >> 

  vQ> yo welcum

    >> 
    >>> there's also an issue with the additional arguments
    >>> supplied after the format: any superfluous arguments are
    >>> ignored (this is not documented, as far as i can see),
    >>> 
    >> 

  MM> I think we should signal an error if there are too many arguments.

  vQ> agree.  but it might be more complex than it appears:

    vQ>     sprintf('%3$s', 1, 2, 3)

    vQ> should *not* complain about too many args, despite just
    vQ> one conversion spec in the format.  

very good point; thanks!

    vQ> Interestingly,

    vQ>     sprintf('%3$s', , , 3) # error: argument is missing,
    vQ> with no default

yes, empty (aka "missing") arguments are not allowed in  sprintf().

    >> Could anyone think of a case where the current behavior
    >> is desirable ?
    >> 

    vQ> well, one scenario might be that one wants to print a collection of
    vQ> items with an arbitrary format, supplied by the users,
    vQ> e.g.

    vQ>     foo = function(fmt) { a = ...  b = ...  ...  s     vQ>
sprintf(fmt, a, b, ...)  ... }

    vQ> without having to examine the format to establish which
    vQ> values are needed.  in the current state, sprintf would
    vQ> use those it would need to use with a particular format,
    vQ> with no undesirable complaints.

ok. you have given good examples which make me revert my
proposal, i.e. continue to not erroring about "too many" arguments. 

    >>> but they *are* evaluated nevertheless, e.g.:
    >>> 
    >>> sprintf('%d', 0, {print(1)}) # "1" # [1]
"0"
    >>> 
    >>> it might be a good idea to document this behaviour.

   MM> actually I think it should be changed to be more strict
   MM> (see above).

as a matter of fact, and the result of many more examples,
I've changed my oppinion and now agree with your original
proposal:

I've just commmited another sprintf() patch which (among more
more important changes) *documents* that all arguments of
sprintf() are evaluated; this actually already entails that
empty / missing arguments are not allowed.

    vq> strict in which sense?  enforce a constraint on the
    vQ> number of arguments to that needed by a specific format?
    vQ> or do you mean evaluation of only those arguments that
    vQ> are needed in a format?  or both?

    vQ> what about:

    vQ>     sprintf('%2$s', {print(1)}, 2) # too many arguments?
    vQ> # should 1 be printed?

    vQ>     sprintf('%2$s', , 2) # too few arguments?  # missing
    vQ> value?  (yes, sprintf is .Internal, but...)


    >> Thank you for the constructive feedback!
    >> 

    vQ> not much to thank for...  certainly, it's the first time my
feedback is
    vQ> called 'constructive'.  i'm making progress, am i not?

indeed!  :-)

Martin Maechler, ETH Zurich

    vQ> btw., thank you for the fixes.  i appreciate your
    vQ> efforts a lot.

    vQ> best, vQ

Martin Maechler wrote:
>
>     vQ>     sptinf('%q%s', 1)
>
>
>     vQ> still suggests that one uses %{f,e,g,a} for numerics,
>     vQ> while %s is pretty much valid, too.  you see, in c
>     vQ> sprintf(buffer, "%s", 1) is destined to cause a
>     vQ> segfault, but in r it works -- so the error message is
>     vQ> slightly misleading, as it suggests %s is *not* valid
>     vQ> for numerics.
>
> yes, but only the error message somewhat suggests that;
> at the moment, I'd like to keep it, since really the user
> *should* think of using the number formats, rather than %s
> {which just calls  as.character(.)}  for numeric arguments
>   
then maybe sprintf('%s', 1) should complain about a format-argument
mismatch?  in c, 1 would be taken to be an address at which a string
starts, but in r you do not have pointers, so this interpretation is
impossible.  if the user *should* use number formats for numerics, %s
should not work.  smells lack of design.

>   MM> I think we should signal an error if there are too many arguments.
>
>   vQ> agree.  but it might be more complex than it appears:
>
>     vQ>     sprintf('%3$s', 1, 2, 3)
>
>     vQ> should *not* complain about too many args, despite just
>     vQ> one conversion spec in the format.  
>
> very good point; thanks!
>
>     vQ> Interestingly,
>
>     vQ>     sprintf('%3$s', , , 3) # error: argument is missing,
>     vQ> with no default
>
> yes, empty (aka "missing") arguments are not allowed in 
sprintf().
>   
be nice and document such items, pliz....  even if all internal
functions have this behavour (do they?), ?sprintf does not say that
sprintf is internal, or that the r wrapper calls an internal so that all
arguments are necessarily evaluated.
>     >> Could anyone think of a case where the current behavior
>     >> is desirable ?
>     >> 
>
>     vQ> well, one scenario might be that one wants to print a collection
of
>     vQ> items with an arbitrary format, supplied by the users,
>     vQ> e.g.
>
>     vQ>     foo = function(fmt) { a = ...  b = ...  ...  s >    
vQ> sprintf(fmt, a, b, ...)  ... }
>
>     vQ> without having to examine the format to establish which
>     vQ> values are needed.  in the current state, sprintf would
>     vQ> use those it would need to use with a particular format,
>     vQ> with no undesirable complaints.
>
> ok. you have given good examples which make me revert my
> proposal, i.e. continue to not erroring about "too many"
arguments.
>   
i did not say i supported that view, however.  it was just an example
where *a* developer might wish sprintf did not complain about wrong
number of arguments.  examples to the opposite effect can easily be
given, but that's not what you asked about.

>     >>> but they *are* evaluated nevertheless, e.g.:
>     >>> 
>     >>> sprintf('%d', 0, {print(1)}) # "1" # [1]
"0"
>     >>> 
>     >>> it might be a good idea to document this behaviour.
>
>    MM> actually I think it should be changed to be more strict
>    MM> (see above).
>
> as a matter of fact, and the result of many more examples,
> I've changed my oppinion and now agree with your original
> proposal:
>
> I've just commmited another sprintf() patch which (among more
> more important changes) *documents* that all arguments of
> sprintf() are evaluated; this actually already entails that
> empty / missing arguments are not allowed.
>   
excellent, thanks.

vQ