This is a live mirror of the Perl 5 development currently hosted at https://github.com/perl/perl5
PERL_SNPRINTF_CHECK(): off by 1 error
authorDavid Mitchell <davem@iabyn.com>
Tue, 27 Jun 2017 08:59:41 +0000 (09:59 +0100)
committerDavid Mitchell <davem@iabyn.com>
Tue, 27 Jun 2017 08:59:41 +0000 (09:59 +0100)
PERL_SNPRINTF_CHECK() is used as part of a wrapper for snprintf()
to check that snprintf didn't return more bytes than the buffer size
given it to it (which should never happen anyway). But it was checking
return_value >= buf_size rather than >. So a spurious panic could ensue
if the formatted string exactly matched the buffer size.

This hadn't been detected before because the old Perl_sv_vcatpvfn_flags()
implementation added lots of fudge factors to the buffer size.

At the same time, change the code in Perl_sv_vcatpvfn_flags() which grows
PL_efloatbuf if its not big enough for float_need:

1) Make it require the buf size to be at least float_need + 1 rather than
just float_need, to accommodate the \0 appended by snprintf() (we don't
strictly need the \0, and a conforming snprintf() implementation should
just return the string without trailing \0 if there isn't room for it,
but its possible an snprintf() out there might stumble).

2) When growing PL_efloatbuf, grow by an extra margin of 0x20, to
reduce the likelihood of multiple reallocs.

perl.h
sv.c
t/op/sprintf.t

diff --git a/perl.h b/perl.h
index 9022114..88c0a3f 100644 (file)
--- a/perl.h
+++ b/perl.h
@@ -1569,7 +1569,7 @@ EXTERN_C char *crypt(const char *, const char *);
  * that should be true only if the snprintf()/vsnprintf() are true
  * to the standard. */
 
-#define PERL_SNPRINTF_CHECK(len, max, api) STMT_START { if ((max) > 0 && (Size_t)len >= (max)) Perl_croak_nocontext("panic: %s buffer overflow", STRINGIFY(api)); } STMT_END
+#define PERL_SNPRINTF_CHECK(len, max, api) STMT_START { if ((max) > 0 && (Size_t)len > (max)) Perl_croak_nocontext("panic: %s buffer overflow", STRINGIFY(api)); } STMT_END
 
 #ifdef USE_QUADMATH
 #  define my_snprintf Perl_my_snprintf
diff --git a/sv.c b/sv.c
index 4576f9c..cb3c1d1 100644 (file)
--- a/sv.c
+++ b/sv.c
@@ -13067,7 +13067,15 @@ Perl_sv_vcatpvfn_flags(pTHX_ SV *const sv, const char *const pat, const STRLEN p
            if (float_need < width)
                float_need = width;
 
-           if (PL_efloatsize < float_need) {
+           if (PL_efloatsize <= float_need) {
+                /* PL_efloatbuf should be at least 1 greater than
+                 * float_need to allow a trailing \0 to be returned by
+                 * snprintf().  If we need to grow, overgrow for the
+                 * benefit of future generations */
+                const STRLEN extra = 0x20;
+                if (float_need >= ((STRLEN)~0) - extra)
+                    croak_memory_wrap();
+                float_need += extra;
                Safefree(PL_efloatbuf);
                PL_efloatsize = float_need;
                Newx(PL_efloatbuf, PL_efloatsize, char);
index 3bde5df..ae481b7 100644 (file)
@@ -773,3 +773,4 @@ e>%vd<   >"version"<    >165.133.153.162.137.150.149<   >perl #102586: vector fl
 >%.001f<   >123.432<   >123.4<   >by tradition, leading zeroes ignored in precison<
 >%.0f<   >[1.2, 3.4]<   >1 REDUNDANT<   >special-cased "%.0f" should check count<
 >%.0f<   >[]<   >0 MISSING<   >special-cased "%.0f" should check count<
+>%53.0f<   >69.0<   >                                                   69<   >#131659<