Bug 48476 - apr_psprintf() does not parse the ll or hh size specifiers
Summary: apr_psprintf() does not parse the ll or hh size specifiers
Status: NEW
Alias: None
Product: APR
Classification: Unclassified
Component: APR (show other bugs)
Version: 1.3.9
Hardware: All All
: P2 regression with 7 votes (vote)
Target Milestone: ---
Assignee: Apache Portable Runtime bugs mailinglist
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2010-01-02 13:38 UTC by squasar
Modified: 2016-09-24 13:17 UTC (History)
4 users (show)



Attachments
Quick patch to make things work again. (548 bytes, patch)
2010-01-02 13:38 UTC, squasar
Details | Diff
Crude solution to the 'll' parsing problem (1.48 KB, patch)
2010-04-07 21:34 UTC, Hyrum Wright
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description squasar 2010-01-02 13:38:17 UTC
Created attachment 24787 [details]
Quick patch to make things work again.

Please note: My actual OS is Mac OS X 10.6, not 10.4. Your OS dropdown could stand to have 10.6 and 10.5 as available options.

Some heavy GDBing of a recurring Subversion crash revealed that apr_psprintf() doesn't appear to parse the %lld specifier format correctly, which causes it not to advance the va_list correctly, which causes it to crash in strlen() when it tries to handle a %s format after the %lld, because it's accessing the wrong pointer. The use of the %lld specifier is defined by APR_OFF_T_FMT on Darwin, making it an internal inconsistency. The check in configure.in on Darwin 10 which makes it use %lld appears to be fallacious; it's definitely unnecessary.

This is a regression inasmuch as it didn't happen in 1.3.8, but the underlying lack of support for the specifier was in 1.3.8 and still appears to be in HEAD.

To fix the issue immediately, do not use %lld as the APR_OFF_T_FMT. To fix it correctly, modify apr_vformatter() to parse the ll size specifier. (And the hh specifier as well).
Comment 1 Hyrum Wright 2010-04-07 21:34:58 UTC
Created attachment 25243 [details]
Crude solution to the 'll' parsing problem
Comment 2 Hyrum Wright 2010-04-07 21:40:17 UTC
I independently uncovered this issue while trying to use APR 1.4.x with Subversion on OS X.  It turns out that we use this construct *a lot*, and this bug causes a number of segfaults.  We've had to explicitly disallow APR 1.4.x with Subversion, which is unfortunate.

I've attached a patch which fixes the parsing problem, but it's a bit crude.  Hopefully this issue can be resolve soon, either by fixing the parser, or by not using the 'll' format specifier.

The thread wherein this is discussed is here:
http://mail-archives.apache.org/mod_mbox/apr-dev/201003.mbox/%3Cb51ffb6f1003100926n22c1dd79id9696972b23a153a@mail.gmail.com%3E
Comment 3 Jeff Trawick 2010-04-08 16:26:37 UTC
On Snow Leopard you must set the arch manually.  For example:

CC="cc -arch i386" ./configure ...

Are you encountering any problems on Snow Leopard if you set the arch
(either to 32-bit or 64-bit)?

>Please note: My actual OS is Mac OS X 10.6, not 10.4. Your OS dropdown could
>stand to have 10.6 and 10.5 as available options.

A work item has been filed for infrastructure to resolve that.
Comment 4 Hyrum Wright 2010-04-13 15:44:11 UTC
(In reply to comment #3)
> On Snow Leopard you must set the arch manually.  For example:
> 
> CC="cc -arch i386" ./configure ...
> 
> Are you encountering any problems on Snow Leopard if you set the arch
> (either to 32-bit or 64-bit)?

Forcing the arch is completely orthogonal to the this issue.  The facts are:

1) configure is special cased to generate the format 'lld' for APR_OFF_T_FMT on Mac OS X.
2) The format parser is broken, in that it does not recognize the format 'lld'

This means that we are putting somebody, somewhere in a potentially broken place.

All that being said, I don't get the error when configuring for 32-bit architecture on Snow Leopard, which is expected.  Instead, I get a bus error when attempting to dynamically load the apr_dbd_sqlite3.so driver file when running the tests.

As for the x86_64 arch, I still get the error, which also makes sense.
Comment 5 Chris Knight 2010-08-20 13:07:13 UTC
As discussed on the apr list, %ll, on MacOSX (and I assume on other 64-bit operating systems) is not recognized as a valid % format. This behavior is incompatible with the libc printf/sprintf/... formatters and causes crashes, such as with the below example. My suggestion is to always consider %ll as a QUAD type, consider %l a QUAD if on 64-bit.

---example---
#include <apr.h>
#include <apr_pools.h>
#include <apr_strings.h>
#include <stdio.h>

int main(int argc, char **argv) {
  apr_pool_t *pool = NULL;
  char *s = "hello world"; u_int64_t v = 12345678;

  apr_pool_initialize(); apr_pool_create(&(pool), NULL);
  printf("%llu%s", v, s); // works
  char *f = apr_psprintf(pool, "%llu%s", v, s); // segfault on strlen
  printf("%s\n", f);
}

---patch---

--- apr_snprintf.c.old	2010-08-20 10:03:41.000000000 -0700
+++ apr_snprintf.c	2010-08-20 10:01:42.000000000 -0700
@@ -813,12 +813,7 @@
              * Modifier check.  Note that if APR_INT64_T_FMT is "d",
              * the first if condition is never true.
              */
-            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 &&
-                 fmt[0] == APR_INT64_T_FMT[0]) ||
-                (sizeof(APR_INT64_T_FMT) > 4 &&
+            if ((sizeof(APR_INT64_T_FMT) > 4 &&
                  strncmp(fmt, APR_INT64_T_FMT, 
                          sizeof(APR_INT64_T_FMT) - 2) == 0)) {
                 /* Need to account for trailing 'd' and null in sizeof() */
@@ -830,7 +825,12 @@
                 fmt++;
             }
             else if (*fmt == 'l') {
-                var_type = IS_LONG;
+		if (fmt[1] == 'l') { // %ll[du] is always quad on 64- & 32-bit
+		    var_type = IS_QUAD;
+		    fmt++;
+		} else if (sizeof(APR_INT64_T_FMT) == 3) { // %l[du] is quad on 64-bit
+		    var_type = IS_QUAD;
+		} else var_type = IS_LONG;
                 fmt++;
             }
             else if (*fmt == 'h') {
Comment 6 William A. Rowe Jr. 2010-08-20 13:50:24 UTC
I'd agree with making 'll' always present a long long int value, but that may
or may not be 8 byte.  This would be platform dependent.

On win32, ll is the equivalent of i64, which is an 8 byte int.
Comment 7 Chris Knight 2010-08-20 13:58:32 UTC
On which platforms is %ll *not* 8 bytes? (Which platforms has sizeof(long long int) != 8?)

On MacOSX 64-bit, sizeof(long long int) == 8 && sizeof (long int) == 8.
Comment 8 William A. Rowe Jr. 2010-08-20 14:03:24 UTC
Chris, that would be for autoconf to determine.

128 bit long long ints on a native 64 bit word environment would not be unreasonabe.
Comment 9 Chris Knight 2010-08-20 14:19:35 UTC
Of course 16-byte (128-bit) ints are in our future but a more significant re-write (adding another IS_* enum, adding code to convert to strin, etc.) will be needed to add support for such ints.

In the meantime, apr_psprintf is incompatible with the system printf in the worst, segfault-causing-kind-of way. "The perfect being the enemy of the good."

Note, also that the current code would incorrectly handle 128-byte ints. (%ll == IS_QUAD == apr_int64_t...)

Of course, if there are 4-byte %ll's or 16-byte %ll's, I'm happy to help write the code to handle.
Comment 10 Hyrum Wright 2011-03-08 09:52:44 UTC
There is now a corresponding Subversion issue which reflect the trouble this is causing for Subversion users on Mac OS X:
http://subversion.tigris.org/issues/show_bug.cgi?id=3829
Comment 11 Steve Northover 2014-04-02 17:21:09 UTC
Test adding a comment.  Sorry for the noise.
Comment 12 franklamoureux 2016-09-24 13:17:36 UTC
This is still true with version 1.5.2, %llu segfaults with this simple test:

ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, "big number: %llu, %s", (long long unsigned) 1474074202886, "some string");