Re: Crash in apr_psprintf(), 1.4.x branch

[Hyrum K. Wright]

On Mar 10, 2010, at 5:50 PM, William A. Rowe Jr. wrote:

> On 3/10/2010 4:45 PM, Hyrum K. Wright wrote:
>> 
>> Digging deeper, it appears to be an error in apr_vformatter() when parsing 
>> the format '%lld'.  I'm running the tests on Mac OS X where APR_OFF_FMT_T is 
>> defined as lld, so this format occurs frequently.  Consequently, I'm also 
>> seeing a failure in testfmt at line 63, where the parser is attempting to 
>> parse APR_OFF_FMT_T, but failing.  Can anybody else replicate this bug?
> 
> Yes, this function isn't maintained on Mac OS/X.  Which is something
> of a surprise, given the number of Mac fans around here!
> 
> The defintions are very clear, %d must handle an (int) sized object,
> %ld must handle a (long) sized object, and %lld must handle any
> long long int sized object.  Patches welcome.
> 
> I plan to patch win32 to accept %I64[ld] because long long type was added
> with VS 7.1, while %ll was not added until the more recent Visual Studio 8.
> I64d goes further back in time to 6.0 or prior, and is how we specified
> the format tag for APR_OFF_T_FMT etc.

Digging even further.  It appears that the parser is attempting to match 
APR_INT64_T_FMT first, which may be of variable size.  On OS X, this is defined 
as 'ld', but the parser mistakenly matches APR_OFF_T_FMT, which is defined as 
'lld' for this case.

Attached is a patch which adds another block which preemptively matches 
APR_OFF_T_FMT.  I've tested it on OS X and a 64-bit Linux system.

-Hyrum


Index: strings/apr_snprintf.c
===================================================================
--- strings/apr_snprintf.c      (revision 921388)
+++ strings/apr_snprintf.c      (working copy)
@@ -810,10 +810,23 @@ APR_DECLARE(int) apr_vformatter(int (*flush_func)(
                 adjust_precision = adjust_width = NO;
 
             /*
-             * Modifier check.  Note that if APR_INT64_T_FMT is "d",
-             * the first if condition is never true.
+             * Modifier check.  Note that if APR_OFF_T_FMT is "d",
+             * the first if condition is never true, and if APR_INT64_T_FMT
+             * is "d", the second if condition is never true.
              */
-            if ((sizeof(APR_INT64_T_FMT) == 4 &&
+            if ((sizeof(APR_OFF_T_FMT) == 4 &&
+                 fmt[0] == APR_OFF_T_FMT[0] &&
+                 fmt[1] == APR_OFF_T_FMT[1]) ||
+                (sizeof(APR_OFF_T_FMT) == 3 &&
+                 fmt[0] == APR_OFF_T_FMT[0]) ||
+                (sizeof(APR_OFF_T_FMT) > 4 &&
+                 strncmp(fmt, APR_OFF_T_FMT, 
+                         sizeof(APR_OFF_T_FMT) - 2) == 0)) {
+                /* Need to account for trailing 'd' and null in sizeof() */
+                var_type = IS_QUAD;
+                fmt += (sizeof(APR_OFF_T_FMT) - 2);
+            }
+            else if ((sizeof(APR_INT64_T_FMT) == 4 &&
                  fmt[0] == APR_INT64_T_FMT[0] &&
                  fmt[1] == APR_INT64_T_FMT[1]) ||
                 (sizeof(APR_INT64_T_FMT) == 3 &&