Re: Problems with atomics in 1.7.2
Hi Yann, Am 14.02.23 um 14:31 schrieb Yann Ylavic: I think we have several issues here: 1. we should set WEAK_MEMORY_ORDERING for anything but __i386__, __x86_64__, __s390__ and __s390x__ 2. we should not use direct 64bit load/store on 32bit systems for atomic operations 3. on 32bit systems the alignment for an uint64_t is usually 4 (not 8 like on a 64bit systems), which hurts (at best) or prevents atomicity even with builtins. For 1. and 2. the generic/mutex case was addressed in r1907541 (apr_atomic_set64() was doing the right thing already), and the builtins case is addressed in r1907637 (hopefully). Windows code is also concerned but I believe it's doing the right thing already by checking _M_X64 (x86_64 CPU) or should we check for _WIN64 too (or APR_SIZEOF_VOIDP >= 8) for WIN32 running on x64? What about the alignment of uint64_t on WIN32, the Interlocked*64() functions seem to require 8-byte alignment so should we fall back to generic/mutex on WIN32 too? For 3., r1907642 should now take care of the alignment to define (or not) HAVE_ATOMIC_BUILTINS64 and HAVE__ATOMIC_BUILTINS64, with a fallback to USE_ATOMICS_GENERIC for cases where it matters. This was tested by looking at the (32bit) assembly generated for different compilers/options/builtins here: https://godbolt.org/z/r4daf6b4v (comment in/out some "__attribute__((aligned(8)))" and/or "&& 0" to see what helps or not). Notably, while both gcc and clang seem to implement the same kind of compare-and-swap loop for _read64 and _set64 with -m32 and the legacy __sync_* builtins, they seem to disagree on whether __atomic builtins should defer to libatomic (thus possibly mutex) when an uint64_t is not 8-byte aligned. gcc will use some x87/FPU instructions (fild/fistp) regardless of the alignment whereas clang will issue calls to libatomic whenever apr_uint64_t is not forcibly 8-byte aligned. Dunno who's correct here (whether x87 instructions work with 4-byte alignment, CPU cacheline crossing and so on..), but this seems to beg for a forced "typedef uint64_t apr_uint64_t __attribute__((aligned(8)));" (an ABI change) or a new API for 64bit atomics (with properly aligned apr_atomic_u64_t) on the APR side anyway to do the right thing on 32bit systems.. For now (after r1907642) this means that compiling a 32bit APR will USE_ATOMICS_BUILTINS64 with gcc and NEED_ATOMICS_GENERIC64 with clang. Does this all work for you? Thanks for all the fixes. I agree with all the changes you did. However, I don't know enough about the windows compilers to comment on that. I would also agree that a 8-byte aligned apr_uint64_t would probably make sense for apr 2.0. On x86 the FPU instructions may work without alignment, but I expect it to have some performance impact. I have applied your commits to Debian's 1.7.2 and there was no test failure during build. I also executed testatomic on powerpc ~ 100 times without problems. Finally it seems that both -march=i[56]86 (but not i[34]86) provide the same atomic builtins too (with gcc), so maybe we could use them instead of the forced generic/mutex implementation (currently) if it's how distros build 32bit APR (per https://bz.apache.org/bugzilla/show_bug.cgi?id=66457). So I think something like this would be fine: I think the intention of the current code was that libapr compiled on any 32bit x86 would run everywhere by default. I don't think this behavior should be changed for apr 1.x, but that is not a strong opinion because 486 is so old. In Debian, we use --enable-nonportable-atomics anyway. Chers, Stefan Index: configure.in === --- configure.in(revision 1907642) +++ configure.in(working copy) @@ -566,6 +566,9 @@ if test "$ap_cv_atomic_builtins" = "yes" -o "$ap_c if test "$ap_cv__atomic_builtins" = "yes"; then AC_DEFINE(HAVE__ATOMIC_BUILTINS, 1, [Define if compiler provides 32bit __atomic builtins]) fi +has_atomic_builtins=yes +else +has_atomic_builtins=no fi AC_CACHE_CHECK([whether the compiler provides 64bit atomic builtins], [ap_cv_atomic_builtins64], @@ -829,10 +832,15 @@ AC_ARG_ENABLE(nonportable-atomics, force_generic_atomics=yes fi ], -[case $host_cpu in - i[[456]]86) force_generic_atomics=yes ;; - *) force_generic_atomics=no - case $host in +[force_generic_atomics=no +case $host_cpu in + i[[34]]86) force_generic_atomics=yes;; + i[[56]]86) + if test "$has_atomic_builtins" != "yes"; then +force_generic_atomics=yes + fi + ;; + *) case $host in *solaris2.10*) AC_TRY_COMPILE( [#include ], ? Regards; Yann.
Problems with atomics in 1.7.2
Hi,
there are a number of problems with the atomics in the latest apr
release. All the results are on Debian sid(unstable).
On powerpc we got a segfault:
Thread 1 (Thread 0xf79226a0 (LWP 458077)):
#0 0x009dc6d0 in mutex_hash (mem=0xffd5f420) at ./atomic/unix/mutex64.c:79
mutex =
mutex =
#1 apr_atomic_set64 (mem=mem@entry=0xffd5f420, val=2) at
./atomic/unix/mutex64.c:104
mutex =
#2 0x00a66ce4 in test_set64 (tc=tc@entry=0xffd5f454,
data=data@entry=0x0) at ./test/testatomic.c:236
y64 = 18434909205434407696
#3 0x00a4ae38 in abts_run_test (ts=ts@entry=0x1f80260,
f=f@entry=0xa66ca0 , value=value@entry=0x0) at ./test/abts.c:186
tc = {failed = 0, suite = 0x1f80230}
ss = 0x1f80230
#4 0x00a693dc in testatomic (suite=0x1f80260) at ./test/testatomic.c:940
No locals.
#5 0x00a4a3b8 in main (argc=, argv=) at
./test/abts.c:444
i =
rv =
list_provided =
suite =
(gdb) p hash_mutex
$1 = (apr_thread_mutex_t **) 0x0
(gdb) q
Fixed in http://svn.apache.org/viewvc?view=revision=1907442 .
Build log at
https://buildd.debian.org/status/fetch.php?pkg=apr=powerpc=1.7.2-1=1675382351=0
On armel, we got a link error:
/usr/bin/ld: ../.libs/libapr-1.so: undefined reference to
`__atomic_fetch_sub_8'
/usr/bin/ld: ../.libs/libapr-1.so: undefined reference to `__atomic_load_8'
/usr/bin/ld: ../.libs/libapr-1.so: undefined reference to
`__atomic_exchange_8'
/usr/bin/ld: ../.libs/libapr-1.so: undefined reference to `__atomic_store_8'
/usr/bin/ld: ../.libs/libapr-1.so: undefined reference to
`__atomic_fetch_add_8'
/usr/bin/ld: ../.libs/libapr-1.so: undefined reference to
`__atomic_compare_exchange_8'
Build log at
https://buildd.debian.org/status/logs.php?pkg=apr=armel=sid
Fixed in http://svn.apache.org/viewvc?view=revision=1907441
However, even with both fixes, there still seem to be problems. On
powerpc, there was the same test failure as in PR 66457:
testatomic: Line 908: Unexpected value
Log at
https://buildd.debian.org/status/fetch.php?pkg=apr=powerpc=1.7.2-2=1675512084=0
If I am not mistaken, on 32 bit architectures you always need to ensure
manually that both halves of a 64 bit value are written atomically. So
code like this
apr_atomic_set64()
...
*mem = val;
seems wrong.
Also, this construct
#if defined(__arm__) || defined(__powerpc__) || defined(__powerpc64__)
#define WEAK_MEMORY_ORDERING 1
#else
#define WEAK_MEMORY_ORDERING 0
#endif
seems to be very fragile. According to the table in
https://en.wikipedia.org/wiki/Memory_ordering , basically all
architectures except x86, amd64, and s390 have weak ordering. So if we
need a list of archs, we must define the list of architectures that have
strong ordering and treat everything else as weak.
BTW, the state of atomics in 1.7.0 has not been perfect, either. I had
applied this workaround to make it work:
https://salsa.debian.org/apache-team/apr/-/blob/debian/1.7.0-8/debian/patches/generic-64bit-atomics.patch
But unfortunately I didn't have time to create a proper patch for upstream.
Cheers,
Stefan
Cleaning up old keys from KEYS?
Hi, one of the debian packagers of apr/httpd has noticed that current gpg refuses to work with some of the old keys in apr's KEYS file because they use MD5: == $ mkdir ring $ chmod 700 ring $ wget https://www.apache.org/dist/apr/KEYS $ gpg2 --homedir ring --import KEYS gpg: keyring `ring/secring.gpg' created gpg: keyring `ring/pubring.gpg' created gpg: ring/trustdb.gpg: trustdb created gpg: key E005C9CB: public key Greg Stein gst...@lyra.org imported gpg: key 13046155: public key Thom May thom...@apache.org imported gpg: key E2226795: public key Justin R. Erenkrantz jerenkra...@apache.org imported gpg: key DE885DD3: public key Sander Striker stri...@apache.org imported gpg: Note: signatures using the MD5 algorithm are rejected gpg: key 10FDE075: no valid user IDs gpg: this may be caused by a missing self-signature gpg: key B55D9977: public key William A. Rowe, Jr. wr...@rowe-clan.net imported gpg: key CC8B0F7E: public key Aaron Bannert abann...@kuci.org imported gpg: key 5C1C3AD7: public key David Reid dr...@apache.org imported gpg: key 42721F00: public key Paul Querna c...@force-elite.com imported gpg: key 9BCFCE2F: public key Garrett Rooney roo...@electricjellyfish.net imported gpg: key 159BB6F8: public key Bradley Nicholes bnicho...@apache.org imported gpg: key 4DAA1988: public key Bojan Smojver bo...@rexursive.com imported gpg: key 08C975E5: public key Jim Jagielski j...@apache.org imported gpg: key 751D7F27: public key Graham Leggett minf...@sharp.fm imported gpg: key 39FF092C: public key Jeff Trawick (CODE SIGNING KEY) traw...@apache.org imported gpg: key 41CEFDE0: public key Stefan Fritsch s...@sfritsch.de imported $ gpg2 --homedir ring --list-keys ring/pubring.gpg pub 1024D/E005C9CB 2002-08-15 uid [ unknown] Greg Stein gst...@lyra.org uid [ unknown] Greg Stein gst...@apache.org uid [ unknown] Greg Stein gst...@collab.net pub 1024D/13046155 2000-10-08 uid [ unknown] Thom May thom...@apache.org uid [ unknown] Thom May t...@planetarytramp.net uid [ unknown] Thom May t...@debian.org uid [ unknown] Thom May t...@tsw.org.uk sub 1024g/C0B77EEF 2000-10-08 pub 1024D/E2226795 1999-09-19 uid [ unknown] Justin R. Erenkrantz jerenkra...@apache.org uid [ unknown] Justin R. Erenkrantz jerenkra...@ebuilt.com sub 2048g/8B626683 1999-09-19 pub 1024D/DE885DD3 2002-04-10 uid [ unknown] Sander Striker stri...@apache.org uid [ unknown] Sander Striker stri...@striker.nl sub 2048g/532D14CA 2002-04-10 pub 4096R/B55D9977 2008-04-09 [expires: 2018-07-07] uid [ unknown] William A. Rowe, Jr. wr...@rowe-clan.net uid [ unknown] William A. Rowe, Jr. wr...@apache.org uid [ unknown] William A. Rowe, Jr. william.r...@springsource.com pub 1024D/CC8B0F7E 2001-10-02 uid [ unknown] Aaron Bannert abann...@kuci.org uid [ unknown] Aaron Bannert aa...@clove.org uid [ unknown] Aaron Bannert aa...@covalent.net uid [ unknown] Aaron Bannert aa...@apache.org sub 2048g/87DB90E0 2001-10-02 pub 1024D/5C1C3AD7 2002-11-17 uid [ unknown] David Reid dr...@apache.org sub 2048g/4CD63851 2002-11-17 pub 1024D/42721F00 2004-01-17 uid [ unknown] Paul Querna c...@force-elite.com uid [ unknown] Paul Querna c...@cyan.com uid [ unknown] Paul Querna pque...@apache.org uid [ unknown] Paul Querna c...@corelands.com sub 2048g/7A2BE310 2004-01-17 pub 1024D/9BCFCE2F 2005-11-22 uid [ unknown] Garrett Rooney roo...@electricjellyfish.net uid [ unknown] Garrett Rooney roo...@apache.org sub 2048g/87DDD6FE 2005-11-22 pub 1024D/159BB6F8 2005-12-07 uid [ unknown] Bradley Nicholes bnicho...@apache.org sub 2048g/431E3F11 2005-12-07 pub 2048R/4DAA1988 2008-08-20 uid [ unknown] Bojan Smojver bo...@rexursive.com sub 2048R/9E49284A 2008-08-20 [expires: 2018-08-18] sub 2048R/CAA19524 2008-08-20 [expires: 2018-08-18] pub 1024D/08C975E5 1999-04-14 uid [ unknown] Jim Jagielski j...@apache.org uid [ unknown] Jim Jagielski j...@jagunet.com uid [ unknown] Jim Jagielski j...@jimjag.com uid [ unknown] Jim Jagielski j...@covalent.net uid [ unknown] Jim Jagielski jim.jagiel...@springsource.com sub 2048g/4CCDB430 1999-04-14 pub 1024D/751D7F27 1999-08-19 uid [ unknown] Graham Leggett minf...@sharp.fm uid [ unknown] Graham Leggett minf...@apache.org sub 2048g/18F4AD9E 1999-08-19 pub 4096R/39FF092C 2010-01-19 uid [ unknown] Jeff Trawick (CODE SIGNING KEY) traw...@apache.org sub 4096R/E4799D69 2010-01-19 pub 4096R/41CEFDE0 2009-07-10 uid [ unknown] Stefan Fritsch s...@sfritsch.de uid [ unknown] Stefan Fritsch s...@debian.org uid [ unknown] Stefan Fritsch s...@apache.org sub 4096R/1AE300C2 2009-07-10 == Can we clean that file up a bit and remove all people who have been in-active for quite some time? Ideally
Re: Pool freelist
On Thursday 04 September 2014 01:07:49, Branko Čibej wrote: On 03.09.2014 23:15, Stefan Fritsch wrote: On Wednesday 03 September 2014 15:37:17, Jim Jagielski wrote: It's now ~6 years later and so wondering if just bypassing the pool freelist is now viable, at least as a compile-time (or allocator) option. I don't see any reason against a non-default compile-time option. Then people could test it more easily and give feedback. I do. Distro packages will tend to pick one or the other option, and if applications have no control over the behaviour, they'll mysteriously behave differently on different platforms (or even different distros of the same OS). Unfortunately without a compile time option, you won't get enough testing. And any significant change in the allocator will cause a problem in some workload. We had, and possibly still have, the same problem with using mmap for pool allocation; I've seen an application mysteriously fail on some Linux distro because it ran out of file handles, but turned out to be hitting a hard limit of the number of mmapped blocks (i.e., pools) per process. Because the distro packager happened to turn mmap for pools on at compile time. I could never reproduce that issue. AFAICT, it is/was a kernel bug in some Linux versions. And the mmap allocator behaves significantly better in general (at least for httpd). Maybe it should be made the default, at least on linux. I don't think it's a good idea to put potential heisenbugs into the code by design. :) -- Brane
Re: Pool freelist
On Wednesday 03 September 2014 15:37:17, Jim Jagielski wrote: It's now ~6 years later and so wondering if just bypassing the pool freelist is now viable, at least as a compile-time (or allocator) option. I don't see any reason against a non-default compile-time option. Then people could test it more easily and give feedback. Cheers, Stefan
Re: svn commit: r1593614 - in /apr/apr/trunk: CHANGES configure.in memory/unix/apr_pools.c
On Thursday 15 May 2014 10:03:25, Ruediger Pluem wrote:
+if (old == 2 pool-in_use_by == apr_os_thread_current()) {
How can old ever be 2?
That was a leftover from a version with a separate state for the pool
being cleared.
+/* cleanups may use the pool */
+return;
+}
+
+if (old != 0)
+pool_concurrency_abort(pool, __func__, old);
Isn't __func__ C99?
Thanks for the review. Both issues fixed in r1595549.
Stefan
Re: documentation for module crypto missing
On Wednesday 07 May 2014 17:32:51, Helmut Tessarek wrote: Hello, The page at http://apr.apache.org/docs/apr-util/1.5/group___a_p_r___util___crypt o.html is empty. Good question. When I generate the docs locally, it has content. Also, where can I find information on when a function was added? That info seems to be missing in the docs, unfortunately. Looking up in the CHANGES or in the svn history seems to be the only way. More to the point: which apr-util release added the apr_bcrypt_encode or the bcrypt code? Changes with APR-util 1.5.0 ... *) apr_password_validate, apr_bcrypt_encode: Add support for bcrypt encoded passwords. The bcrypt implementation uses code from crypt_blowfish written by Solar Designer solar openwall com. apr_bcrypt_encode creates hashes with $2y$ prefix, but apr_password_validate also accepts the old prefix $2a$. PR 49288. [Stefan Fritsch]
Re: digest functions and validation - part 2
Am Freitag, 9. Mai 2014, 14:26:26 schrieb Helmut Tessarek: Thanks for the answer. On 09.05.14 3:22 , Stefan Fritsch wrote: No. But which password hashing algorithmis are used/supported by apr_password_validate() is rather unrelated to which digest functions are made available with a public interface. For password hashing, apr-util has been supporting bcrypt since version 1.5. It's great to have bcrypt available, but I hoped that Ulrich Drepper's sha256 and sha512 implementations would be part as well. His code is public domain, so it shouldn't be a license issue. At the moment, bcrypt is the only safe choice really. Doesn't this seem a bit strange to you? IMO Ulrich's functions are standard these days and APR should include them, just my 2 cents. bcrypt has a smaller speed-up from normal CPUs to GPUs than sha256crypt/sha512crypt. This means if you tune the rounds to make bcrypt give you the same level of security as sha*crypt, bcrypt is faster. For the normal login on a server, this is not really relevant (100ms or 500ms, who cares). But in a web server, speed is very relevant. You may need to do 1s of password checks per second. You cannot increase the number of rounds arbitrarily. Therefore bcrypt is more secure than sha*crypt for web servers. I don't see any reason to add a less secure algorithm. And I didn't choose scrypt because I felt it was still rather new when I included bcrypt support in apr. However, there is a contest for password hashing alorithms [1]. If there is some winner, and after the winner has seen some scrunity from cryptoanalysts, it may make sense to include that into apr. [1] https://password-hashing.net/ What is missing is the support in httpd 2.2's htpasswd to generate hashes with bcrypt. And even in 2.4, bcrypt is not yet used by default. Both things should be changed, but are entirely unrelated to apr. Yes, the httpd project seems stangely slow at times. It almost reminds me of IBM, where I worked for 17 years. (You have a great idea, which takes you half a day to implement (which you do), but then it takes 2 years (and a lot of administrative processes) to get it into a product - if at all.) It's more like openssl. Considering the huge userbase of httpd and its importance for the internet's infrastructure, there are very little developer ressources available.
Re: digest functions and validation - part 2
On Mon, 5 May 2014, Helmut Tessarek wrote: I really would like to know, if and why the httpd and apr developers think that md5 and sha1 are safe choices to be used for hashing passwords. No. But which password hashing algorithmis are used/supported by apr_password_validate() is rather unrelated to which digest functions are made available with a public interface. For password hashing, apr-util has been supporting bcrypt since version 1.5. What is missing is the support in httpd 2.2's htpasswd to generate hashes with bcrypt. And even in 2.4, bcrypt is not yet used by default. Both things should be changed, but are entirely unrelated to apr. Cheers, Stefan
Re: pool owner debugging idea
Am Freitag, 2. Mai 2014, 19:39:52 schrieb Branko Čibej: Is there anyway this trylock could be conditional? For example, could we add some sort of flag or whatever that would implement this on specifically created pools? We could do that, but I don't really see a use case. I was imagining a debug option that is globally switchted on by a configure option. And this would not be used in production, at least not unless one is hunting a specific bug. A pool debug option that does something like that would be welcome. But it shouldn't be turned on by default, or pool allocation performance is likely to go down the drain. It turns out one does not need a mutex for this at all. apr_atomic_cas32() is enough. I agree that this should not be enabled by default, but I still hope that it is fast enough to be included in --enable-maintainer-mode. Does anyone have any suggestions how to call such an option? Maybe --enable-pool-concurrency-checks or --enable-pool-concurrency-debug ? What use did you have in mind? Maybe you were looking for some really thread-safe pools that use a mutex to serialize accesses? In this case, the mutex would be enabled by some flag to apr_pool_create(). There is PR 51392 requesting that. If you think that would be useful, that could be something for apr 1.6. I'm very sceptical of such an approach. Serializing access to a pool like that is likely to be a band-aid for bad API design; if you need a pool shared across threads, e.g., to implement a cache, then serializing allocation is likely to be the least of your concerns, and not at all the same as serializing access. For the latter you need explicit locking, and doing that at the pool level is probably wrong. In other words, a feature like that would only help someone who takes a very naïve (or lazy) approach to software architecture. We really don't have to support that. :) I think in some cases where a special pool is used very seldomly, it could be an acceptable solution. But it would come with the danger that people would just switch to this type of pool instead of fixing the actual bugs.
Re: [PATCH] APR pool scalability and efficiency
Hi Stefan, On Sunday 11 May 2014 04:23:38, Stefan Fuhrmann wrote: The first and more important one was getting an OOM error because we hit the mmap region count limit on Ubuntu (mmap seems to be enabled in the system libs at least since 12.04 LTS). Using mmap has the advantage that free memory can be given back to the OS much more often, which is a big advantage for httpd, especially with mpm prefork. Therefore I enabled use of mmap in Debian/Ubuntu. Its root cause was a large data structure (many small allocations) being built up in a single pool. That times multiple threads / concurrent requests exceeded the ~0.5GB limit (~64k regions x 8kB). [apr-pool-growth.*] interesting. From my experience, Linux seems to merge adjacent anonymous mappings. How does the process map look when it goes OOM? Does subversion create lots of file-backed mappings that are interspersed with anon mappings? Or does the OOM happen when one thread unmaps its memory, causing a lot of fragmentation? In a related scenario, we would allocate a large buffer (e.g. 64kB) in a temporary pool, clear the pool and then create another pool that is small (few allocations) but relatively long-lived. The APR allocator would then tend to reuse the large node for the new pool - wasting quite a bit of memory. [apr-allocator-efficiency.*] I have thought about this case, too. A different approach would be to split the large node, take what was requested for the current allocation and put the rest back into the free list as a smaller node. This works only with mmap/munmap and not with malloc/free, of course. Not sure which method is better. One could do both, I guess. Attached are patches against apr/trunk to both issues plus the corresponding commit messages. Neither should change the behavior of common pool usage but rather improve worst-case behavior. Cheers, Stefan
Re: [PATCH] APR pool scalability and efficiency
On Sunday 11 May 2014 16:07:32, Stefan Fuhrmann wrote:
On Sun, May 11, 2014 at 10:28 AM, Stefan Fritsch s...@sfritsch.de
wrote:
Its root cause was a large data
structure (many small allocations) being built up in a single
pool. That times multiple threads / concurrent requests
exceeded the ~0.5GB limit (~64k regions x 8kB).
[apr-pool-growth.*]
interesting. From my experience, Linux seems to merge adjacent
anonymous mappings. How does the process map look when it goes
OOM?
Does subversion create lots of file-backed mappings that are
interspersed with anon mappings? Or does the OOM happen when one
thread unmaps its memory, causing a lot of fragmentation?
Nope. Although the original code contains a few temporary
subpools, the following code already exhibits the problem
when run in the SVN test suite:
static void test_pools(apr_pool_t *pool)
{
int i;
for (i = 0; i 1000; ++i)
{
if (i % 10 == 0)
printf(%d\n, i);
apr_palloc(pool, 100);
}
}
Output:
...
500
510
libsvn: Out of memory - terminating application.
*** Program received signal SIGABRT (Aborted) ***
So, it seems that the individual regions do not get combined.
IIRC from earlier debug output, they are actually adjacent, though.
Strange. With 32bit userspace, this works for me up to 39490775 for
the upper bound, which is near the expected limit. With 64bit, higher
values work, too.
$ cat /proc/sys/vm/max_map_count
65530
$ cat /proc/sys/kernel/randomize_va_space
2
$ uname -a
Linux k 3.14-1-amd64 #1 SMP Debian 3.14.2-1 (2014-04-28) x86_64
GNU/Linux
I only see one region in /proc/$pid/maps
OTOH, if I enable the shiny new --enable-allocator-guard-pages, I get
lots of fragmentation and failure at around 250, also as expected.
Maybe your kernel has some randomization feature that my kernel lacks.
Though as far as I could find out, randomize_va_space == 2 means that
heap+mmap randomization is enabled.
-- Stefan^2.
[Once again showing that numbering one's name resolves ambiguity.]
Though counting to two is not sufficient to make it unique ;)
Re: No v1.6.x branch?
Am Montag, 21. April 2014, 15:26:59 schrieb Graham Leggett: New features seem rather easy to follow using trunk/CHANGES and-or a diff of the include directories. For finer detail, it is much easier for a person who cares to record revision numbers of desired feature backports in trunk/STATUS than for everyone to have to touch an additional branch for bug fixes, and then double check before we actually release it since we've had issues with that in the past. The shortcut you're describing is technical debt. Instead of the person who wants the fix and/or feature committed doing the work of committing it to various branches that work falls to others who are probably not in a position to make the backport as carefully or test it as thoroughly. This introduces stability problems we just don't want. It's a trade-off between two kinds of technical debts. 1) If you create branch 1.n+1 late, you have to go through trunk to see if no new features have been missed. 2) If you create branch 1.n+1 early, you have to go through 1.n to see that no bug fixes have been missed. Missing bug fixes in unreleased 1.x branches definitely has been a problem in the past. I prefer missing features to re-introduced bugs and therefore option 1). A way to have both would be a pre-commit hook that (once the 1.n+1 branch is created) rejects commits to 1.n that do not reference a commit in 1.n+1 in their commit message or merge info. But I don't know if this can be easily done in svn. Or if people would even want that. Another idea to limit the work to search for missing bug fixes would be to release new features much more often. Basically you would branch 1.n+1 when you want to commit the first feature to it. After that, one could limit the life-time of 1.n. Maybe only one more release or only 3 more months, or something like that.
pool owner debugging idea
Hi, it's currently rather hard to debug issues caused by pools being used by different threads. The current pool debugging options don't really work if pool ownership changes during runtime (as int mpm event), require specific apr_pool_owner_set() calls, and apr compiled with pool debugging is significantly slower than normal apr. So, I had the following idea for a lightweight debugging help: All pool operations are protected by mutexes. But in order to get the mutex, the operations use apr_thread_mutex_trylock(). If there is any contention while getting the mutex, we abort(). This would only catch threading issues during relatively high load when there is actual contention. But the mutex operations should be relatively cheap comparted to the full blown pool debugging. So maybe this feature could be even enabled by default if compiled in maintainer mode. Do you think this would be a good idea? Cheers, Stefan
Re: pool owner debugging idea
Am Donnerstag, 1. Mai 2014, 06:37:04 schrieb Jim Jagielski: I would really appreciate subversion's input on this... in many many ways, they push APR to its limits, esp related to pools. I don't know if subversion uses threads heavily. I suspect that the pattern of pools being used by different worker threads is unique to mpm_event. Is there anyway this trylock could be conditional? For example, could we add some sort of flag or whatever that would implement this on specifically created pools? We could do that, but I don't really see a use case. I was imagining a debug option that is globally switchted on by a configure option. And this would not be used in production, at least not unless one is hunting a specific bug. What use did you have in mind? Maybe you were looking for some really thread-safe pools that use a mutex to serialize accesses? In this case, the mutex would be enabled by some flag to apr_pool_create(). There is PR 51392 requesting that. If you think that would be useful, that could be something for apr 1.6. On May 1, 2014, at 5:16 AM, Stefan Fritsch s...@sfritsch.de wrote: Hi, it's currently rather hard to debug issues caused by pools being used by different threads. The current pool debugging options don't really work if pool ownership changes during runtime (as int mpm event), require specific apr_pool_owner_set() calls, and apr compiled with pool debugging is significantly slower than normal apr. So, I had the following idea for a lightweight debugging help: All pool operations are protected by mutexes. But in order to get the mutex, the operations use apr_thread_mutex_trylock(). If there is any contention while getting the mutex, we abort(). This would only catch threading issues during relatively high load when there is actual contention. But the mutex operations should be relatively cheap comparted to the full blown pool debugging. So maybe this feature could be even enabled by default if compiled in maintainer mode. Do you think this would be a good idea? Cheers, Stefan
Re: apr-util v1.6 branch?
On Thursday 06 June 2013, Graham Leggett wrote: On 06 Jun 2013, at 5:24 PM, Jeff Trawick traw...@gmail.com wrote: It makes sense to have it once we have changes that someone wants to release in 1.x but which can't be released in 1.5.x. (I guess you already have such changes in mind so go for it ;) ) Indeed I have :) As we've had some trouble in the past keeping such next-1.x branches up to date (mostly minor stuff like doxygen improvements that skipped 1.latest), I think it is best to defer until actual use. I am ready to backport http://svn.apache.org/r1489517, http://svn.apache.org/r1489520 and http://svn.apache.org/r1490248, but have nowhere to backport it to. I am also in favor of waiting with the branching until there is some momentum to release 1.6.0. Backporting stuff to multiple branches is annoying. Maybe put a 'waiting for apr-util 1.6' section into trunk STATUS with the list of revisions?
Re: [PATCH] unix/sockaddr.c: return error when call_resolver fails and *sa is 0
On Thursday 02 May 2013, Jan Kaluža wrote: Attached patch fixes it by returning APR_EGENERAL in this case. Thanks. Committed with small tweaks as http://svn.apache.org/r1478905 trunk http://svn.apache.org/r1478909 1.5 http://svn.apache.org/r1478911 1.4 Cheers, Stefan
Re: 1.4.next next weekend or so?
On Sunday 28 April 2013, Rainer Jung wrote: On 26.04.2013 21:18, Jeff Trawick wrote: Some warnings during make check but probably not new: test/testlockperf.c:229:17: warning: variable 'lockname' set but not used test/testmmap.c:122:18: warning: variable 'rv' set but not used test/testfile.c:739:18: warning: variable 'rv' set but not used test/testfile.c:925:18: warning: variable 'rv' set but not used test/testnames.c:272:9: warning: variable 'hadfailed' set but not used test/echod.c:116:18: warning: variable 'rv' set but not used test/sockperf.c:206:18: warning: variable 'rv' set but not used Backported Looking through all open issues of 2012 and 2013 I find some which might be not to much work: Bug 54892 - Free without malloc (apr_pool_create_unmanaged_ex) Trunk only, fixed. Bug 54779 - apr_sockaddr_info_get() can return APR_SUCCESS when it fails leading to apache 2.4.4 segv Fixed backported. Bug 54093 - Adding support for 'll' (long long) to snprintf New feature, should wait for 1.5 IMHO Bug 54032 - Default prefix incorrect in configure script Possible trunk backports: Bug 53609 - Apache hangs with terminated signal 6 Was already backported. Bug 52785 - testall fails if pool debug is enabled Needs new API, must wait for 1.5. But API may change again to allow mpm-event to work with pool debugging, therefore waiting with backport to 1.5. Cross compilation stuff: Bug 53425 - Make more cross compiler friendly at configure time Bug 53426 - apr_hints: Fixed underquoted definitions Bug 53427 - apr_network: Fixed underquoted definitions added a default to TRY_RUN for cross-compiling Bug 53428 - apr_threads: Fixed underquoted definitions added a default to TRY_RUN for cross-compiling Other bugs that need more analysis or work on a patch: Bug 53996 - shmget fails on duplicate ftok Bug 53906 - make check failures testsockets testime test_exp_get_lt apr_mcast_hops Bug 54643 - Application exception from Apache 2.4.3 (with APR 1.4.6) Regards, Rainer
Re: [VOTE] Release APR-util 1.5.2
On Saturday 30 March 2013, Jeff Trawick wrote: Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ Shortcuts to CHANGES: http://apr.apache.org/dev/dist/CHANGES-APR-UTIL-1.5.2 http://apr.apache.org/dev/dist/CHANGES-APR-UTIL-1.5 autoconf version: 2.69 (same as 1.5.1) libtool version: 2.4.2 (1.5.1 had 2.4.2 Debian-2.4.2-1.1 +/-1 [ ] Release APR-util 1.5.2 as GA +1 tests pass on Debian sid/x86, sid/sparc, lenny/armel
Re: [VOTE] Release APR-util 1.5.2
On Sunday 31 March 2013, Rainer Jung wrote: - testall sometimes fails in testpass on Linux and often fails in testcrypto when testing nss passphrase (see below). Have you any details about the testpass failures? There have been numerous changes and fixes there in 1.5.2, so that would be interesting. OTOH, you also had those failures with 1.5.1. Cheers, Stefan
Re: [CONCEPT-PATCH] require --enable-unsupported-freetds to support FreeTDS with apr-util 1.5.x
On Saturday 30 March 2013, Jeff Trawick wrote: Make sense for post-1.5.2? This doesn't affect non-autoconf builds, and I think the RPM build would be broken. Any other missing pieces? Works for me. I ok with including it post-1.5.2 or in 1.5.2. BTW, if you have time to TR today, go ahead. I won't until tomorrow.
Re: time for APU 1.5.2?
On Monday 11 March 2013, Jeff Trawick wrote: That sounds like a good idea. I can RM in a few days. Is there anything important missing? AFAICS, the memcached changes proposed by Joshua Marantz add a new field to a public struct and therefore need to wait for 1.6. I could TR tomorrow or on Monday.
Re: [PATCH] Reduce apr_[i|l|off_t_]toa memory footprint for common cases
On Monday 25 March 2013, Christophe JAILLET wrote: As a first step, I noticed that apr_itoa, apr_ltoa, apr_off_t_toa could be tweaked to require less memory for some common cases. The attached patch reduces memory use for small values, that is to say for strings that fit in 8 bytes (including NULL) Looks like a reasonable optimization to me. Cheers, Stefan
Re: bug 54572 -- move struct crypt_data allocation to heap
On Saturday 16 March 2013, Jeff Trawick wrote: This would be good to resolve in 1.5.2. Has anyone else evaluated this? I'm suspicious of the use of a global pool in the reporter's patch vs. just using malloc() directly. I guess the reason for using the pool is that the allocator may have suitable buffers lying around, but you need one for the pool and one for the structure instead of just getting one from malloc(). I haven't tried any performance tests yet. An alternative would be using apr_allocator_alloc() directly (with the global pool's allocator). Creating a sub-pool seems more overhead than necessary. Not sure what is better, malloc() or apr_allocator_alloc().
Re: bug 54572 -- move struct crypt_data allocation to heap
On Saturday 23 March 2013, Jeff Trawick wrote: On Sat, Mar 23, 2013 at 12:01 PM, Stefan Fritsch s...@sfritsch.de wrote: On Saturday 16 March 2013, Jeff Trawick wrote: This would be good to resolve in 1.5.2. Has anyone else evaluated this? I'm suspicious of the use of a global pool in the reporter's patch vs. just using malloc() directly. I guess the reason for using the pool is that the allocator may have suitable buffers lying around, but you need one for the pool and one for the structure instead of just getting one from malloc(). I haven't tried any performance tests yet. An alternative would be using apr_allocator_alloc() directly (with the global pool's allocator). Creating a sub-pool seems more overhead than necessary. Not sure what is better, malloc() or apr_allocator_alloc(). No good way to get to global_pool/global_allocator from outside apr_pools.c AFAICT I think malloc() wins here... You are right. What about http://svn.apache.org/r1460243 Anyone has a system using CRYPTD to test this? Cheers, Stefan
Re: bug 54572 -- move struct crypt_data allocation to heap
On Sunday 24 March 2013, Eric Covener wrote: Anyone has a system using CRYPTD to test this? AIX anyone? Poked around and CRYPTD stuff doesn't show up in my builds on AIX. HP-UX uses CRYPTD according to google...
Re: apr_proc_detach and chdir
On Friday 18 January 2013, Javier Castillo II wrote: Looking into the code for apr_proc_detach(int daemonize), it appears that chdir(/) is being run unconditionally. Was there a specific reason that / is chosen? I guess it's not much of a big deal, because you can chdir into the directory it should be in after you detach, however, why is this forced into the function to begin with? I guess that this is to avoid troubles when unmounting file systems. If the daemon would not change its working directory explicitly, the file system where it was started could not be unmounted until the daemon is killed. The assumption is probably that / will not need to be unmounted. And it is guaranteed to exist. At the moment, I am fine with using getcwd and chdir to get back to where I started, however it does seem counter-intuitive. So, I am guessing that this is the expected practice when calling this function, correct? Any insight would be great and any other methods to use with this function to get around it would be appreciated. Many daemons don't need a specific working directory, so it would seem quite likely that many daemon authors would not consider the unmounting problem and forget doing a chdir(). Therefore it makes sense to do it automatically.
Re: Question about httpd memory buffer alignment
On Friday 11 January 2013, Zhe Zhang wrote: I would like httpd to use 4K-aligned buffers so that I can do direct I/O on the html files being served. Could anyone tell me which file I need to modify? I've already changed APR_BUCKET_BUFF_SIZE to 8192 so at least the buffer size is multiple of 4K. There is --enable-allocator-uses-mmap when compiling apr. It will cause the apr allocator to do page aligned allocations. But I think that would only be the first step because apr will always put some management struct (apr_memnode_t and/or node_header_t) at the start of the allocated space. This would somehow have to be changed so that the management stuff is placed somewhere else.
Re: Debian: apu-config and BDB
On Thursday 22 November 2012, Ben Reser wrote: Since the change that was made in reaction to this bug report: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=622081 apu-config on Debian has avoided returning the BDB library from --libs. The bug suggests that BDB is related to the DBM support in APR Util. This is actually not the case. They are two separate database interfaces supported by APR Util as can be seen by the two separate API pages for them: http://apr.apache.org/docs/apr-util/1.4/group___a_p_r___util___d_b_ d.html vs http://apr.apache.org/docs/apr-util/1.4/group___a_p_r___util___d_b_ m.html You are confusing two things here. Apr-util dbm is an abstraction for key-value-type database libraries of which BDB is one possible provider. Apr-util dbd (note the different order of the letters than bdb) is an abstraction for sql database engines (e.g. mysql, postgresql, ...). The bug also says that it thinks this dependency is useless. It may be useless for some projects that aren't using the bdb features of the library. Those that are using the bdb features will have problems with their build system as a result of this change. For example. Subversion would not detect BDB on Debian based systems due to this change (until I made a change to our build system to work around this). Apr-util provides a full abstraction for the dbm backends. There is no need and no way for a consumer of the apr-util dbm API to directly call functions of the backend library and make them interact with apr- util's dbm functions. Therefore, on Linux it is unnecessary for any consumer of the apr-util dbm API to directly link to the dbm libraries (except for linking statically). It is enough that apr-util links to the dbm libraries (like DBD). Therefore it is generally a good idea on Linux to not output the dbm libraries by default. So, it's more a platform specific thing than something related to a specific project. Subversion is (or was?) using apr-util's detection code of BDB without using apr-util's dbm API. This is an abuse of apu-config. There is no reason at all why subversion would need to use the same version of BDB as apr-util does. In fact, this interdependency had caused problems in Debian in the past. Ultimately there are two failures here. 1) You shouldn't be changing apu-config's output in order to remove dependencies that are useless for a particular project. Rather you should patch that's project's use of apu-config to filter those things out. 2) Passing --dbm-libs to get BDB libraries is wrong. I'd strongly advise that this change be reverted and that the packages that end up with BDB dependencies that are undesirable be fixed. Alternatively, you could add --avoid-bdb and --bdb-libs flags to apu-config. I'd suspect that something like that would be accepted upstream by APR-UTIL. No, it is not. BDB is one dbm provider. I'd urge the Debian project not to unilaterally take it upon itself to change the default behavior of a configuration tool like this. We will make changes to make packages work together flawlessly. I don't think this particular change is a very intrusive one. But if the current subversion still has a problem with it, it may be a good idea to make it pass --dbm-libs by default.
Re: svn commit: r1403239 - in /apr/apr-util/branches/1.3.x: ./ CHANGES buckets/apr_brigade.c
On Monday 29 October 2012, rpl...@apache.org wrote: Author: rpluem Date: Mon Oct 29 10:26:10 2012 New Revision: 1403239 URL: http://svn.apache.org/viewvc?rev=1403239view=rev Log: Merge r1402870 from trunk: Remove duplicated logic in apr_brigade_puts(). Thanks for backporting the commits. But do we really need to update apr-util 1.3 and 1.4, considering that 1.5.1 has been released?
Re: [VOTE] Release apr-util 1.5.1
Hi, thanks for everyone voting/commenting. While there seem to be some open issues, none are regressions WRT 1.4.1. We have binding +1 votes from minfrin, rjung, covener, sf, plus a non- binding +1. The vote passes. I will proceed with the release real soon now (TM). Cheers, Stefan
Re: [VOTE] Release apr-util 1.5.1
On Friday 07 September 2012, Stefan Fritsch wrote: [ ] Release apr-util 1.5.1 as GA +1 (tested on Debian sid)
Re: crypt_r versus macosx
On Thursday 06 September 2012, Benson Margulies wrote: I'm trying to build httpd 2.4.3 on OSX Mountain Lion with mod_session_crypto. I'm failing. I think that the reason is the following error from the configure process for apr-util: configure:36291: checking for crypt_r configure:36291: gcc -o conftest -g -O2 -DDARWIN -DSIGPROCMASK_SETS_THREAD_MASK -no-cpp-precomp conftest.c 5 Undefined symbols for architecture x86_64: _crypt_r, referenced from: _main in cc3WYSLM.o Sure enough, no such API on osx. Am I out of luck? No, configure should then continue without crypt_r support. Does configure abort after this check? If so, can you post more of its output? E.g. the last 100 lines before it aborts?
Re: apr_dbd_freetds
On Wednesday 22 August 2012, Graham Leggett wrote: On 22 Aug 2012, at 7:18 PM, Nick Kew wrote: PR 53666 tells us apr_dbd_freetds doesn't work with Sybase, and very probably never did. The reporter attaches a patch, but it's one I'm not happy with, even if I had access to any FreeTDS backend to test-drive (which I don't). The basic objection is that FreeTDS doesn't support prepared statements, and the emulation in the driver opens big security issues. We've had a bit of a thread on the subject on dev@httpd. Is anyone in a position to take up the baton on FreeTDS? If not, perhaps it's time we dropped that driver in favour of the ODBC one. Am I right in understanding that a user of the freetds driver could realistically use the ODBC driver instead? (I am assuming this is Windows). If so, I would be in favour of deprecating the freetds driver and dropping the driver in v2.0, as a driver that doesn't support prepared statements suffers higher security risks. I think the FreeTDS driver should either emulate prepared statements by using a known secure escaping function from FreeTDS. If that does not exist, it should be removed. Also, apr_dbd_escape currently returns the unchanged string for FreeTDS. I would be more comfortable if it returned NULL (or if there was a way to return ENOTIMPL).
Re: apu-util DBD back-ends
On Thursday 16 August 2012, olli hauer wrote: My my intention comes from the FreeBSD ports system. We do not use the bundled apr/apr-util from apache, there we use apr / apr-util as own port. (Perhaps with apr-1.4.6 and apr-util-1.5 in a view weeks). This separation allows us to use an actual apr/apr-util and the user to build apr with different backends, but then we do not have enough information in the apache port to construct build params and dependency lists ... I don't understand what problem you are trying to solve. The apache port should not need any different build params or dependency lists depending on which dbd backends are available. Having a dependency on the apr-util port and calling apu-1-config should be enough. Or do you have to list all of apr-util's dependencies in the apache port, too? As workaround I can implement this together with a function which lists DBD backends as local patch but I prefer upstream support. If there is interest I can spend some more time to extend apr/apr-util.
[VOTE] Release apr-util 1.5.1
Hi, here comes the next try. Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ Full CHANGES are here: http://apr.apache.org/dev/dist/CHANGES-APR-UTIL-1.5 http://apr.apache.org/dev/dist/CHANGES-APR-UTIL-1.5.1 +/-1 [ ] Release apr-util 1.5.1 as GA
Re: [VOTE] Release apr-util 1.5.0
On Wednesday 08 August 2012, Gregg Smith wrote: 1 warning crypt_blowfish.c .\crypto\crypt_blowfish.c(894) : warning C4244: '=' : conversion from 'unsigned long' to 'char', possible loss of data That's a false positive. count can only be between 4 and 31 at this point. IMHO the compiler should be able to recognize that. I have a single patch (attached) to deal with all these issues. This is same in apr-util 1.4.1 and patch should work for both 1.4.x 1.5.x I cannot say anything about the other issues you mentioned. Has anyone who is familiar with the Windows toolchain looked at the patch?
releasing 1.5.1 (was: [VOTE] Release apr-util 1.5.0)
On Wednesday 22 August 2012, William A. Rowe Jr. wrote: The code exists already in a licensable form as part of the spec, I believe. http://tools.ietf.org/html/rfc3492#appendix-C This can be made more usable with pools, etc with a pretty trivial effort. Before I start, though, From my reading, the license should be compatible. But since you seem too busy and nobody else has stepped up, I would like to postpone that punycode thing and go ahead with 1.5.1 soon (next week-end or earlier). As soon as punycode support is there, we can bump to 1.6, version numbers are cheap. Cheers, Stefan
Re: [VOTE] Release apr-util 1.5.0
Thanks for the detailed tests. On Friday 10 August 2012, Rainer Jung wrote: If you don't need to release before, I would like to fix the LDADD use over the weekend, but it is not a show stopper. It seems I will be busy in the next few days. It is unlikely that I TR 1.5.1 before next Wednesday. Another nice to have would be an expat update to 2.1.0. Has anyone looked into this before and knows how much work this is? Another nice to have fix would be support for newer Berkley DB versions (PR 53684).
Re: [VOTE] Release apr-util 1.5.0
On Tuesday 07 August 2012, Eric Covener wrote: On Mon, Aug 6, 2012 at 4:24 PM, Stefan Fritsch s...@sfritsch.de wrote: Hi, Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ It includes many bug fixes and new built-in bcrypt support. Full CHANGES are here: http://apr.apache.org/dev/dist/CHANGES-APR-UTIL-1.5 +/-1 [ ] Release apr-util 1.5.0 as GA r1357772 moved some code that depended on apu_config.h to apr_passwd.c which didn't include it. On AIX this triggers a compile-time error but on other platforms it probably causes APU to use crypt() with our own mutex. Thanks for catching this. I will TR 1.5.1 in a few days. But if somebody else with a platform that is not Linux, AIX, Netware, or Windows wants to do some testing first, that would be appreciated. Maybe Solaris or FreeBSD, anyone? It seems on Netware, apr-util 1.5 only builds with the svn version of APR 1.4 because of some build system changes. Looking at the CHANGES file, I think releasing apr 1.4.7 as well may be a good idea. If anyone wants to add some final fixes there, go ahead. In particular, it would be nice if those Netware build-system changes would get a CHANGES entry, too. Cheers, Stefan
Re: [VOTE] Release apr-util 1.5.0
On Tuesday 07 August 2012, Jeff Trawick wrote: But if somebody else with a platform that is not Linux, AIX, Netware, or Windows wants to do some testing first, that would be appreciated. Maybe Solaris or FreeBSD, anyone? BTW, did anyone test on Windows yet? Gregg and Steffen have thankfully responded to my mail from a few days ago and have reported that it compiles ok.
[VOTE] Release apr-util 1.5.0
Hi, Tarballs/zipfiles are at http://apr.apache.org/dev/dist/ It includes many bug fixes and new built-in bcrypt support. Full CHANGES are here: http://apr.apache.org/dev/dist/CHANGES-APR-UTIL-1.5 +/-1 [ ] Release apr-util 1.5.0 as GA
apr-util 1.5 on Windows and Netware?
Hi, has anyone tried to compile apr-util 1.5 on Windows or Netware in the last two weeks? I intend to TR as soon as I get positive feed-back, or in a few days if there is no response. Cheers, Stefan
apr-util 1.5 ?
Hi, I would like to get apr-util 1.5 out soonish. It adds bcrypt support for password hashes. If anyone wants to add something else, go ahead and commit. Cheers, Stefan
Re: Choosing a stronger password hash algorithm
On Sunday 01 July 2012, William A. Rowe Jr. wrote: On 6/29/2012 4:23 AM, Stefan Fritsch wrote: Therefore I will go ahead and add bcrypt support to apr-util which will be a big improvement for the 95% of users who don't need FIPS. It sounds to me that after we spent a great deal of time to make APR largely library agnostic, you are insisting on binding us to a specific library. I would be very hostile to that. No. As I have mentioned in one of the other mails, I prefer importing a suitably licensed version of the source code. What I currently have is http://people.apache.org/~sf/apr_bcrypt_password_support.diff diffstat: build.conf |2 crypto/apr_md5.c| 145 --- crypto/apr_passwd.c | 196 ++ crypto/crypt_blowfish.c | 902 ++ crypto/crypt_blowfish.h | 27 + include/apr_md5.h |5 If you are talking about a plugable, client-agnostic approach which the user can ignore the details of how apr was built, that would be fine. Before throwing code at APR, please post a patch, because the group might largely be in agreement. The delta to configure.in and the headers is probably sufficient for now. But if your solution is to add more mandatory dependencies or make apr less flexible, it would be met with resistance. No new dependency, no configure.in change. Instead I add two files from the crypt_blowfish distribution. I intend to make some benchmarks with the assembler version, but it says it was written for Pentium 1, so I am not so sure that it will greatly improve performance on current processors. If it is actually significantly faster than the pure C version, I would add it plus the necessary build logic. That would mean one additional file (there is only an assembler version for i386). The diff of crypt_blowfish.c versus upstream is one line. So importing new versions if there happen to be any upgrades should be easy. The user should remain largely oblivious whether the pw crypt used lives in bcrypt or openssl or the kernel, and the resulting pw data should be always portable between different builds of apr. This was the logic behind the sha1, md5 and other common password schemes. The crypt scheme was an exception, one which can be resolved even on win32 and hpux with the use of openssl's crypt implementation or linking to fcrypt. Platform or library binding dependent pw data takes us back to 1999. Yes. That's why I was sceptical about using apr_crypto for this task, too. After all, apr_crypto is still optional. Cheers, Stefan
Re: Choosing a stronger password hash algorithm
On Monday 02 July 2012, Ben Laurie wrote: FWIW, I am not super-keen on this particular move. Whilst bcrypt is certainly an improvement, I am wary of relying on Blowfish - it is not a mainstream cipher and is not subject to the kind of scrutiny that, say, AES or SHA-2/3 are. If we are going to change, then we should change to a mechanism that is subject to ongoing cryptanalysis. bcrypt has the advantage that it currently does not give much speed-up of GPUs versus CPUs. So brute-forcing is more difficult than e.g. for glibc's sha256 or sha512 based algorithms. And we can't just arbitrarily increase the number of rounds because that would make httpd prone to DoS attacks. My rationale for bcrypt is here: http://mail-archives.apache.org/mod_mbox/apr- dev/201206.mbox/%3C201206232242.42669.sf%40sfritsch.de%3E Your comments on that would be welcome. Due to Poul-Henning Kamp's declaration that md5crypt is insecure, there is some renewed interest in this field. Maybe there will be a new algorithm soon that is both difficult to brute-force on GPUs and based on something standard like AES or SHA*. Maybe we could add bcrypt for now and if something better appears, then add that as well? Cheers, Stefan
Re: Choosing a stronger password hash algorithm
On Tuesday 26 June 2012, William A. Rowe Jr. wrote: On 6/24/2012 3:34 AM, Stefan Fritsch wrote: On Sunday 24 June 2012, Graham Leggett wrote: Ideally, like we have a generic synchronous encryption API, we should have a generic hash API too, so that the user can use whatever hash that the underlying toolkit provides. I rather like the fact that you can use htpasswd on one system and use the result on another system, regardless of the operating system. If we are willing to give that up, we may just make htpasswd use the more advanced schemes offered by the system's crypt() function. You misunderstand. The algorithms need to exist in the apr crypt implementation. Passing some salt string like $6$ to crypt() is just a different way to call a library. And on FIPS compliant systems, the system's password hashes are likely also FIPS compliant. But this approach would not work for platforms like Windows that don't have crypt(). We might choose to provide 'fallback' implementations if the are absent. This gets you to things like FIPS-140-2 implementations when APR is correctly configured and built, without the hassle of validating our own implementation. We aren't put in a position of implementing the assembly language optimized flavor, because the library vendor has already done this. Nobody is talking about crypt(), although generic implementations of crypt() are offered by openssl and likely from gnutls etc. There is some confusion here between crypt() the function and crypt the algorithm (i.e. DES-crypt). On Linux and BSDs, crypt() supports more secure algorithms than DES-crypt. Also, using what is available in the crypto libraries would limit us to PKCS5/PBKDF2, which is the only password hashing algorithm supported by openssl (AFAICS). Since PBKDF2 is based on sha256, though, it scales relatively well on GPUs. Of course, we could also implement other schemes on top of some cryptographhic hash or cipher provided by the crypto libraries. But I would rather avoid that because it's a lot of work to verify the result. Take a quick glance through these two drafts; http://csrc.nist.gov/publications/drafts/800-107/Draft_Revised_SP800-107.pdf http://csrc.nist.gov/publications/drafts/800-118/draft-sp800-118.pdf I think we may be dwelling too much on computation time. The larger issue is better seeding. An illicitly obtained password file of hashed p/w's will always be a risk to the accounts, irrespective of computation time. What is critical is the seeding such that duplicate passwords are not obvious, and that passwords recycled on other services don't demonstrate the same hash (ergo, collision) value. The first paper doesn't mention password hashing at all. The second one mentions stretching only to make creation of rainbow tables more difficult. But increasing computation time by stretching is also important for simple brute force attacks without rainbow tables. And the reason why the md5crypt author has recently declared his algorithm insecure is the too low computation time and nothing else. The salting definitely can be improved. htpasswd only uses 2^32 different values on a given libc. htdbm only uses the timestamp as salt. Apart from that I think that we should do both things: Add a good non- FIPS compliant algorithm that is built into APR and a FIPS compliant variant (whatever that is). The former is straightforward but the latter isn't. For example with PBKDF2, you have to define an output format because none is defined yet. Also, one must choose a suitable size of the resulting key. And I don't really know if PBKDF2 is actually FIPS compliant. Then there is the question of the API: Would we add a replacement function for apr_password_validate() that also takes an apr_crypto_driver_t argument? Or would we make some apr_password_set_driver() function that sets the driver to be used by apr_password_validate()? Do we also need a way to disable non-FIPS compliant algorithms? Therefore I will go ahead and add bcrypt support to apr-util which will be a big improvement for the 95% of users who don't need FIPS. I would be willing to help anyone who wants to add a FIPS compliant scheme, but I am not going to do the necessary research myself. And I don't think we should delay the release of a new apr-util with bcrypt support for a FIPS compliant scheme that may or may not be added in the future. Cheers, Stefan
Re: Choosing a stronger password hash algorithm
On Sunday 24 June 2012, Graham Leggett wrote: On 24 Jun 2012, at 12:01 AM, Stefan Fritsch wrote: Openssl is not required, neither for apr nor for httpd. I propose to import either crypt_blowfish or scrypt into apr, just like apr contains some foreign sha1 and md5 code. This way we would not get an additional external dependency. APR-util has a crypto library to deal with this exact problem - the need for low level crypto functions without having to tightly bind ourselves to one toolkit over another, or import code. With the formal move by the Redhat people towards NSS as a shared crypto API, this becomes more important as time goes by. Ideally, like we have a generic synchronous encryption API, we should have a generic hash API too, so that the user can use whatever hash that the underlying toolkit provides. I rather like the fact that you can use htpasswd on one system and use the result on another system, regardless of the operating system. If we are willing to give that up, we may just make htpasswd use the more advanced schemes offered by the system's crypt() function. Also, using what is available in the crypto libraries would limit us to PKCS5/PBKDF2, which is the only password hashing algorithm supported by openssl (AFAICS). Since PBKDF2 is based on sha256, though, it scales relatively well on GPUs. Of course, we could also implement other schemes on top of some cryptographhic hash or cipher provided by the crypto libraries. But I would rather avoid that because it's a lot of work to verify the result.
Choosing a stronger password hash algorithm
On Thursday 21 June 2012, Ben Laurie wrote: Then the question is: what scheme? Here are some design criteria I think would be useful. 1. Use something from the SHA-2 family - SHA-512 would seem fine to me. The sha message digests are designed to be fast and to be easily implemented in hardware. In general, the speed-up when going from general purpose CPU implementations to parallel implementations using GPU or specialized hardware seems to be relatively high. Therefore I think what is generally used as sha256_crypt or sha512_crypt is not the optimal choice. [1] has some thoughts about choosing a password hashing algorithm. 2. Use a very large salt - disk space is not at a premium for password stores! Agreed. 64bit should be absolute minimum, more would be better. 3. Use quite a lot of rounds,. The number should be configurable so that we can adjust the cost without breaking backward compatibility. 4. Use something that is hard to optimise in hardware (ideally). bcrypt [1] and scrypt [2] seem to be much more difficult to port to hardware or GPUs than sha256/512_crypt [3-5]. We can't make the operation too expensive on normal CPUs or we create a DoS problem if someone does lots of requests with wrong passwords. Therefore I think choosing an algorithm that does not scale well on more specialized hardware is good. Both bcrypt and scrypt can be adjusted in how much CPU time to use. scrypt can also be adjusted in how much RAM it uses. bcrypt uses a 128bit salt, AFAICS scrypt can use arbitrary salt lengths. Bcrypt has seen a lot more review than scrypt, therefore I am somewhat in favor of bcrypt. Crypt_blowfish [6] is an implementation with a very liberal license that we could use. Scrypt has a 2-clause BSD license which would also be OK. Opinions? Cheers, Stefan [1] http://static.usenix.org/event/usenix99/provos.html [2] http://www.tarsnap.com/scrypt.html [3] http://stackoverflow.com/a/6807375 [4] http://www.openwall.com/lists/john-dev/2012/05/14/1 [5] http://www.openwall.com/lists/john-dev/2012/05/14/4 [6] http://www.openwall.com/crypt/
Re: Choosing a stronger password hash algorithm
On Saturday 23 June 2012, William A. Rowe Jr. wrote: On 6/23/2012 3:42 PM, Stefan Fritsch wrote: bcrypt [1] and scrypt [2] seem to be much more difficult to port to hardware or GPUs than sha256/512_crypt [3-5]. We can't make the operation too expensive on normal CPUs or we create a DoS problem if someone does lots of requests with wrong passwords. Therefore I think choosing an algorithm that does not scale well on more specialized hardware is good. Both bcrypt and scrypt can be adjusted in how much CPU time to use. scrypt can also be adjusted in how much RAM it uses. bcrypt uses a 128bit salt, AFAICS scrypt can use arbitrary salt lengths. Bcrypt has seen a lot more review than scrypt, therefore I am somewhat in favor of bcrypt. Crypt_blowfish [6] is an implementation with a very liberal license that we could use. Scrypt has a 2-clause BSD license which would also be OK. Opinions? We already have integration points to openssl, why add yet another dependency? Seems foolish. Openssl is not required, neither for apr nor for httpd. I propose to import either crypt_blowfish or scrypt into apr, just like apr contains some foreign sha1 and md5 code. This way we would not get an additional external dependency.
Re: svn commit: r1308135 - in /apr/apr-util/branches/1.4.x: ./ CHANGES crypto/apr_crypto.c
On Sunday 08 April 2012, William A. Rowe Jr. wrote: On 4/4/2012 5:09 PM, Graham Leggett wrote: This type of comment embarrasses both the project and the ASF. If you want to discuss an issue, please discuss the issue. As soon as emails start getting abusive, EOT. Graham Leggett, The issue is that you have an history of unprofessional conduct, both with vehemently pushing malformed API's and commit changes, blocking sensible corrections to your ill-formed designs, and now without even proper log entries, consistently without a semblance of design sensibility. You truly should be ashamed. Bill, I think your criticism is completely out of proportion to the issue at hand, namely not including the trunk revision number in the log message. Other commiters produce broken log messages now and then, too. And the apr-ldap interface is broken. So what. Now it has been removed from apr trunk. Things like that happen. Other committers have introduced broken APIs, too. And both individual people and the project as a whole learn from their mistakes. Cheers, Stefan
Re: backports/merges
On Monday 09 April 2012, Rainer Jung wrote: Do you disagree with the procedure and/or my attempt to describe the normal way this is handled? No, I agree and I think it is more useful to include the CHANGES entry in the backport commit than to split it in a second commit. At least that's what I tried to do in the past influenced by following the list and commit messages. Sometimes the CHANGES entry either is forgotten during the backport commit or postponed by a differing personal preference and is then added soon as a separate commit which I think is less useful but still acceptable. +1
Re: modes of discussion
On Sunday 08 April 2012, Jeff Trawick wrote: Some of the behavior here is embarrassing. Yelling about not following procedures that nobody else even follows as stated, threatening, whining, crudity or at least making fun of objections to crudity, extreme defensiveness, etc. do not reflect well on us or our ability to do anything together. Even without staking out extreme positions, attempts to communicate concerns in the simplest manner can elicit very strong emotional reactions which feed the frenzy. Some less active participants must see it as a poor statement on the human condition that there can be such fireworks about such a simple little project. We need to try harder to avoid the extreme offensive and defensive positions and acknowledge that e-mail is a limited tool in the best of circumstances, and that some of us are enigmas to each other even in the best of circumstances. +1
Re: [VOTE] Release apr-util 1.4.2
On Monday 02 April 2012, Graham Leggett wrote: On 01 Apr 2012, at 11:06 PM, Stefan Fritsch wrote: Sorry for being late, but -1 Works fine if linked dynamically with DSOs. But for the static build, httpd segfaults in mod_ssl (backtrace at the bottom of the mail). Since 1.4.1 doesn't compile with 1.4.1 at all, it's hard to say if this is a regression. Have you tried that? I am not sure if it's apr-util's fault or if some of the other libraries produce some strange conflict. Configure was: ./configure --with-crypto --with-openssl=/usr --with-nss=/usr --with- dbm=db --with-ldap=yes --with-berkeley-db=/usr --with-pgsql --with- mysql --with-sqlite3 --with-freetds --with-odbc --enable-maintainer- mode --prefix=/usr/local/Aprutil1 --enable-nonportable-atomics -- enable-threads --enable-allocator-uses-mmap --with-apr=/usr/bin/apr- config CFLAGS= -gdwarf-2 -g3 -Wextra -Wno-unused-parameter -Wno- missing-field-initializers -Wno-sign-compare -Werror=declaration- after-statement --disable-util-dso configure: WARNING: unrecognized options: --enable-maintainer-mode, -- enable-nonportable-atomics, --enable-threads, --enable-allocator-uses- mmap Would it be possible to reduce this down to a reproducible set of configure options? The unrecognised options seem strange, as does --with-apr=/usr/bin/apr-config (shouldn't it be apr-1-config?). What options did you use to try and build httpd? Given that static builds have in the past not worked at all, this isn't a regression. I just discovered static builds of DSO code were broken on apr-trunk and have been for some time, so any improvement is a step forward. When I wrote that, I could reproduce it also with a minimal configure command, as long as mod_session_crypto was loaded. But I can't reproduce it now. Maybe it was fixed by r1308135. If configured statically but without openssl, it doesn't compile: crypto/apr_crypto.c: In function 'apr_crypto_get_driver': crypto/apr_crypto.c:227:5: error: 'else' without a previous 'if' make[1]: *** [crypto/apr_crypto.lo] Fehler 1 This is definitely a bug, fixed in r1308318. Thanks. That was the main reason for my -1.
Re: [VOTE] Release apr-util 1.4.2
On Sunday 25 March 2012, Graham Leggett wrote:
Fixed in apr-util is the static build of the apr_crypto interface.
Full CHANGES are here:
https://svn.apache.org/repos/asf/apr/apr-util/tags/1.4.2/CHANGES
+/-1
[ ] Release apr-util 1.4.2 as GA
Sorry for being late, but
-1
Works fine if linked dynamically with DSOs.
But for the static build, httpd segfaults in mod_ssl (backtrace at the
bottom of the mail). Since 1.4.1 doesn't compile with 1.4.1 at all,
it's hard to say if this is a regression. Have you tried that? I am
not sure if it's apr-util's fault or if some of the other libraries
produce some strange conflict.
Configure was:
./configure --with-crypto --with-openssl=/usr --with-nss=/usr --with-
dbm=db --with-ldap=yes --with-berkeley-db=/usr --with-pgsql --with-
mysql --with-sqlite3 --with-freetds --with-odbc --enable-maintainer-
mode --prefix=/usr/local/Aprutil1 --enable-nonportable-atomics --
enable-threads --enable-allocator-uses-mmap --with-apr=/usr/bin/apr-
config CFLAGS= -gdwarf-2 -g3 -Wextra -Wno-unused-parameter -Wno-
missing-field-initializers -Wno-sign-compare -Werror=declaration-
after-statement --disable-util-dso
configure: WARNING: unrecognized options: --enable-maintainer-mode, --
enable-nonportable-atomics, --enable-threads, --enable-allocator-uses-
mmap
If configured statically but without openssl, it doesn't compile:
crypto/apr_crypto.c: In function 'apr_crypto_get_driver':
crypto/apr_crypto.c:227:5: error: 'else' without a previous 'if'
make[1]: *** [crypto/apr_crypto.lo] Fehler 1
According to CHANGES, the only change is to fix static build, and it
fails at that. Therefore the -1.
Program terminated with signal 11, Segmentation fault.
#0 0xf5c34140 in ?? ()
(gdb) bt full
#0 0xf5c34140 in ?? ()
No symbol table info available.
#1 0xf73062f0 in int_free_ex_data (class_index=10, obj=0x8db1848,
ad=0x8db1860) at ex_data.c:522
mx = 1
i = optimized out
item = 0x8dc5558
ptr = optimized out
storage = 0x1
#2 0xf7306063 in CRYPTO_free_ex_data (class_index=10, obj=0x8db1848,
ad=0x8db1860) at ex_data.c:592
No locals.
#3 0xf73a023f in x509_cb (operation=3, pval=0xffb07870,
it=0xf7465d3c, exarg=0x0) at x_x509.c:113
ret = 0x8db1848
#4 0xf73a573c in asn1_item_combine_free (pval=optimized out,
it=optimized out, combine=0) at tasn_fre.c:173
tt = optimized out
seqtt = optimized out
ef = optimized out
cf = optimized out
aux = optimized out
asn1_cb = optimized out
i = optimized out
#5 0xf73a5957 in ASN1_item_free (val=0x8db1848, it=0xf7465d3c) at
tasn_fre.c:71
No locals.
#6 0xf73a0465 in X509_free (a=0x8db1848) at x_x509.c:141
No locals.
#7 0xf5d8eff3 in ssl_pphrase_Handle (s=0xf67df3e8, p=0xf67a7018) at
ssl_engine_pphrase.c:275
key_id = 0xf5a07f08 localhost:8534:RSA
using_cache = optimized out
mc = 0xf7768ce0
sc = 0xf5ab3788
pServ = 0xf5ab5388
cpVHostID = 0xf596c8f8 localhost:8534
szPath = /home/stf/apache/perl-
framework/t/conf/ssl/ca/asf/certs/server.crt\000õ\000 \000\000hÈ\226õ
'\230õ\220Ã\177öì×n÷ty°ÿ`\bz÷å´~öø×n÷ôßz÷@'\230õå´~öð{°ÿ\000-
conference/x-
cooltalk\t\t\t\tice\000\000.mov\000\000ia\000\000in\000\000\000\000
w3d fgd swa\000\000signature+xml\000\000xml...
pPrivateKey = optimized out
asn1 = optimized out
ucp = 0x8dc52f5 óÙmvgç\231\001
length = optimized out
pX509Cert = 0x8db1848
bReadable = optimized out
aPassPhrase = 0xf596c8b8
nPassPhrase = 0
nPassPhraseCur = 134815315
cpPassPhraseCur = 0x6 Address 0x6 out of bounds
nPassPhraseRetry = optimized out
nPassPhraseDialog = 0
nPassPhraseDialogCur = 10114
bPassPhraseDialogOnce = 0
cpp = optimized out
i = 0
j = optimized out
algoCert = 1
algoKey = optimized out
at = 1
an = 0xf5d9ebfa RSA
pkey_mtime = 0
rv = optimized out
#8 0xf5d85f76 in ssl_init_Module (p=0xf7787018, plog=0xf67ad018,
ptemp=0xf67a7018, base_server=0xf67df3e8)
at ssl_engine_init.c:322
mc = optimized out
sc = 0xf5b89a50
s = optimized out
#9 0x0808c399 in ap_run_post_config (pconf=0xf7787018,
plog=0xf67ad018, ptemp=0xf67a7018, s=0xf67df3e8) at config.c:105
pHook = optimized out
n = optimized out
rv = 0
#10 0x0806d3ad in main (argc=9, argv=0xffb09b24) at main.c:765
c = 68 'D'
showcompile = 0
showdirectives = 0
confname = 0xffb0b20d /home/stf/apache/perl-
framework/t/conf/httpd.conf
def_server_root = 0xffb0b1e8 /home/stf/apache/perl-
framework/t
temp_error_log = 0x0
error = optimized out
process = 0xf77890a8
pconf = 0xf7787018
plog = 0xf67ad018
ptemp = 0xf67a7018
pcommands = 0xf67e1018
opt = 0xf67e10b8
rv = optimized out
mod = 0x80c0d6c
Re: Using autoconf/automake/libtool
On Friday 16 March 2012, Graham Leggett wrote: Automake may not have done the trick in 2002, but it's now 2012, and I think it's fair that we looked at this again. FWIW, my last experiences with automake were about 2 years ago and I agree with Greg that it's extremely painful as soon as you try to do something more complex.
Re: svn commit: r1240475 - in /apr/apr/branches/1.4.x: ./ include/apr_errno.h
On Saturday 04 February 2012, Graham Leggett wrote: On 04 Feb 2012, at 11:47 AM, s...@apache.org wrote: URL: http://svn.apache.org/viewvc?rev=1240475view=rev Log: Reserve a range of USERERR values for HTTPD --- apr/apr/branches/1.4.x/include/apr_errno.h (original) +++ apr/apr/branches/1.4.x/include/apr_errno.h Sat Feb 4 09:47:57 2012 @@ -158,6 +158,8 @@ APR_DECLARE(char *) apr_strerror(apr_sta * Subversion - Defined ranges, of less than 100, at intervals of 5000 * starting at an offset of 5000, e.g. * +5000 to 5100, +1 to 10100 + * + * Apache HTTPD - +2000 to 2999 */ #define APR_OS_START_USERERR(APR_OS_START_STATUS + APR_OS_ERRSPACE_SIZE) /** Would it be possible to have this as reserved for the application, rather than reserved for httpd? Ideally APR shouldn't have any httpd specific code in it, however this error range would be useful for other external apps, including httpd. Read the full comment in the file (the diff context is too small). There is a huge range of error codes reserved for the application. Out of this range, subversion already had some ranges reserved. Now HTTPD has some other range reserved. There is still plenty of space for other applications.
Re: svn commit: r1240475 - in /apr/apr/branches/1.4.x: ./ include/apr_errno.h
On Saturday 04 February 2012, William A. Rowe Jr. wrote: Read the full comment in the file (the diff context is too small). There is a huge range of error codes reserved for the application. Out of this range, subversion already had some ranges reserved. Now HTTPD has some other range reserved. There is still plenty of space for other applications. It still doesn't make much sense. 1. you can't add reservations in apr 1.x - that would be a semantic change reserved for apr 2.x 2. it's not apr_*.h job to document userspace (and yes, if svn ranges are defined here, that too was an error). These surely belong in http_/ap_*.h header space, no? I guess the comment in apr_errno.h is only informational. In most cases it doesn't matter if two applications use the same error codes. But it won't hurt to have the ranges used by the two biggest apr consumers documented, even if it's not an API guarantee. But I don't care much. Just remove the whole comment block if you think it's a bad idea.
Re: Win32 apr-util's crypto, cprypto_nss and crypto_openssl now works
On Wed, 7 Dec 2011, Graham Leggett wrote: On 7 Dec 2011, at 08:58, Mladen Turk mt...@apache.org wrote: Fixed the 1.4.x branch win32 crypto API and modules (Mostly by adding missing build files and one API bug) They can now compile using Mozilla's xulrunner SDK and any OpenSSSL SDK. We could reconsider re-tagging 1.4.0 (or just making 1.4.1) Happy to just retag 1.4.0, if no objections will do so later this afternoon. I've got a report that apr-util/ldap 1.3.12 does not build on Debian unstable currently, presumably due to toolchain changes. If you wait another 6 hours or so, I could check if that also affects 1.4, and see if I can fix it for 1.4.1. Cheers, Stefan
Re: Win32 apr-util's crypto, cprypto_nss and crypto_openssl now works
On Wednesday 07 December 2011, Graham Leggett wrote: On 7 Dec 2011, at 13:54, Stefan Fritsch s...@sfritsch.de wrote: I've got a report that apr-util/ldap 1.3.12 does not build on Debian unstable currently, presumably due to toolchain changes. If you wait another 6 hours or so, I could check if that also affects 1.4, and see if I can fix it for 1.4.1. Cool, will wait for later tonight. False alarm, it works ok with 1.4.0. Bojan has already fixed that issue in r1154889. Cheers, Stefan
Re: svn commit: r1207680 - in /apr/apr/trunk: crypto/apr_crypto_nss.c dbd/apr_dbd_sqlite3.c dbm/apr_dbm_sdbm.c test/testreslist.c util-misc/apr_thread_pool.c
On Tuesday 29 November 2011, William A. Rowe Jr. wrote: On 11/28/2011 5:32 PM, William A. Rowe Jr. wrote: On 11/28/2011 5:29 PM, Graham Leggett wrote: On 29 Nov 2011, at 01:21, William A. Rowe Jr. wrote: - rv = apr_sdbm_nextkey(dbm-file,rd); + apr_sdbm_nextkey(dbm-file,rd); pkey-dptr = rd.dptr; pkey-dsize = rd.dsize; apr-trunk contains the following explanation for this, I understand it's intended (sf?): /* * XXX: This discards any error but apr_sdbm_nextkey currently returns * XXX: an error for the last key */ Interesting. Good if that's the only case; it still seems odd ;-) The error handling of apr_sdbm_nextkey is broken, but I don't remember the details. FWIW, isn't this invalid on some builds? Don't you need to cast the ignored function rv to null? Seems to be trading your warning for someone elses' warning. E.g. (void)apr_sdbm_nextkey(dbm-file, rd); IMHO this should only be necessary if apr_sdbm_nextkey() had attribute warn_unused_result.
Re: svn commit: r1204998 - /httpd/httpd/trunk/server/util_expr_eval.c
On Tuesday 22 November 2011, j...@apache.org wrote: Author: jim Date: Tue Nov 22 14:02:25 2011 New Revision: 1204998 URL: http://svn.apache.org/viewvc?rev=1204998view=rev Log: Hello. Let's compile again. Modified: httpd/httpd/trunk/server/util_expr_eval.c Modified: httpd/httpd/trunk/server/util_expr_eval.c URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_expr_ev al.c?rev=1204998r1=1204997r2=1204998view=diff == --- httpd/httpd/trunk/server/util_expr_eval.c (original) +++ httpd/httpd/trunk/server/util_expr_eval.c Tue Nov 22 14:02:25 2011 @@ -29,6 +29,8 @@ #include apr_lib.h #include apr_fnmatch.h +#include limits.h /* for INT_MAX */ + /* we know core's module_index is 0 */ #undef APLOG_MODULE_INDEX #define APLOG_MODULE_INDEX AP_CORE_MODULE_INDEX Interesting. On Unix and Netware, apr.h already includes limits.h. But on Windows it doesn't. Shouldn't that be identical on all platforms? Such differences don't make writing portable code any easier.
Re: buildbot failure in ASF Buildbot on apr-util-branch-1.4-fbsd
On Saturday 15 October 2011, build...@apache.org wrote: The Buildbot has detected a new failure on builder apr-util-branch-1.4-fbsd while building ASF Buildbot. Full details are available at: http://ci.apache.org/builders/apr-util-branch-1.4-fbsd/builds/3 Buildbot URL: http://ci.apache.org/ Buildslave for this Build: bb-fbsd Build Reason: scheduler Build Source Stamp: [branch apr/apr-util/branches/1.4.x] 1183737 Blamelist: sf I don't think this has anything to do with my commit. I have only changed a .c file but there is something broken in the build system: Makefile, line 50: Could not find /usr/home/buildbot/slave6/apr- util-branch-1.4-fbsd/build/build/rules.mk make: fatal errors encountered -- cannot continue BUILD FAILED: failed compile_2 sincerely, -The Buildbot
Re: Has been APR initialized?
On Tuesday 20 September 2011, Antonio Vieiro wrote: Is there any way to know if APR has already been initialized? I'm working on two modules that use APR, and I'd like any of this to initialize/terminate APR in case of need. Just call apr_initialize/apr_terminate in each modules. Only the first call to apr_initialize and the n-th call to apr_terminate will do something (if apr_initialize has been called n times). I guess this should be made clearer in the docs.
Re: APR threadpool and memory pool weirdness
On Wednesday 27 July 2011, Cosmin Luță wrote: I have a rather strange use case for APR which leads to quick memory increase on Linux (at least): a program creates (in a loop) memory pools which are passed to tasks in a threadpool. Each task will call apr_pool_destroy on its pool (received as an argument). AFAICS due to the sleeps, you destroy pools at a max rate of 10/25ms while you create pools as fast as possible. Your memory usage will increase until all pools are created.
Re: testreslist failures / thread-safety issues
On Sunday 22 May 2011, Jeff Trawick wrote:
Can those people who frequently hit the testreslist failures
please check if this commit helps? From a powerpc Debian build
host, it looks promising, but I don't have direct access to that
machine and can't do repeated tests there.
no luck on Windows 7
apr-util 1.3.x, gcc, still fails
apr trunk, Visual C++ 2008 Express, still fails
Seven:~/svn/apr-util-1.3.x-xx/test$ ./testall -v testreslist
testreslist : /Line 258: expected 10, but saw 20
FAILED 1 of 1
Failed TestsTotal FailFailed %
===
testreslist 1 1100.00%
Can you try the attached patch in addition? Is this failure sporadic
or always?
Unfortunately, there are also other thread-safety issues in
apr/apr- util. Using hellgrind on testreslist found these
issues:
- unsafe read of pool-child in apr_pool_destroy. This one
worries me. If a subpool is destroyed in one thread while the
pool itself is destroyed in another thread, it can happen that
apr_pool_destroy is called again for the subpool. This would
likely lead to a crash. Any ideas on how to fix this without
sacrificing too much performance?
- unsafe read of allocator-max_index (PR 48535, likely not
critical)
- unsafe read of _myself-thd_cnt in thread_pool_cleanup()
(likely not critical; but there seem to be other thread-safety
issues in that file, at least the access of thd_high is also
unsafe)
The last one is fixed by the attached patch. I no longer think that it
is no problem.
diff --git a/util-misc/apr_thread_pool.c b/util-misc/apr_thread_pool.c
index 006370b..b6a5297 100644
--- a/util-misc/apr_thread_pool.c
+++ b/util-misc/apr_thread_pool.c
@@ -333,9 +333,13 @@ static apr_status_t thread_pool_cleanup(void *me)
_myself-terminated = 1;
apr_thread_pool_idle_max_set(_myself, 0);
+apr_thread_mutex_lock(_myself-lock);
while (_myself-thd_cnt) {
+apr_thread_mutex_unlock(_myself-lock);
apr_sleep(20 * 1000); /* spin lock with 20 ms */
+apr_thread_mutex_lock(_myself-lock);
}
+apr_thread_mutex_unlock(_myself-lock);
apr_thread_mutex_destroy(_myself-lock);
apr_thread_cond_destroy(_myself-cond);
return APR_SUCCESS;
@@ -367,14 +371,13 @@ APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
apr_pool_cleanup_register(tp-pool, tp, thread_pool_cleanup,
apr_pool_cleanup_null);
+apr_thread_mutex_lock(tp-lock);
while (init_threads) {
/* Grab the mutex as apr_thread_create() and thread_pool_func() will
* allocate from (*me)-pool. This is dangerous if there are multiple
* initial threads to create.
*/
-apr_thread_mutex_lock(tp-lock);
rv = apr_thread_create(t, NULL, thread_pool_func, tp, tp-pool);
-apr_thread_mutex_unlock(tp-lock);
if (APR_SUCCESS != rv) {
break;
}
@@ -384,6 +387,7 @@ APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
}
--init_threads;
}
+apr_thread_mutex_unlock(tp-lock);
if (rv == APR_SUCCESS) {
*me = tp;
Re: testreslist failures / thread-safety issues
On Sunday 22 May 2011, Jeff Trawick wrote: Can those people who frequently hit the testreslist failures please check if this commit helps? From a powerpc Debian build host, it looks promising, but I don't have direct access to that machine and can't do repeated tests there. no luck on Windows 7 apr-util 1.3.x, gcc, still fails apr trunk, Visual C++ 2008 Express, still fails Seven:~/svn/apr-util-1.3.x-xx/test$ ./testall -v testreslist testreslist : /Line 258: expected 10, but saw 20 FAILED 1 of 1 Failed TestsTotal FailFailed % === testreslist 1 1100.00% :-( Thanks for testing, though.
testreslist failures / thread-safety issues
On Sunday 22 May 2011, s...@apache.org wrote: Author: sf Date: Sun May 22 20:10:42 2011 New Revision: 1126207 URL: http://svn.apache.org/viewvc?rev=1126207view=rev Log: Fix thread unsafe pool usage. This is a potential culprit for the occasional testreslist failures. Can those people who frequently hit the testreslist failures please check if this commit helps? From a powerpc Debian build host, it looks promising, but I don't have direct access to that machine and can't do repeated tests there. Unfortunately, there are also other thread-safety issues in apr/apr- util. Using hellgrind on testreslist found these issues: - unsafe read of pool-child in apr_pool_destroy. This one worries me. If a subpool is destroyed in one thread while the pool itself is destroyed in another thread, it can happen that apr_pool_destroy is called again for the subpool. This would likely lead to a crash. Any ideas on how to fix this without sacrificing too much performance? - unsafe read of allocator-max_index (PR 48535, likely not critical) - unsafe read of _myself-thd_cnt in thread_pool_cleanup() (likely not critical; but there seem to be other thread-safety issues in that file, at least the access of thd_high is also unsafe) Cheers, Stefan
Adding gcc attributes to function declarations
Hi, I think it would be a good idea to add some more gcc attribute annotations to our header to mark parameters that must not be NULL, etc. We already have some of these for format string. In order to not break other compilers, I thought to just include #if !defined(__GNUC__) #define __attribute__(x) #endif at the top. Or would it be better to have something like #if defined(__GNUC__) #define APR_NONNULL(x) __attribute__((nonnull(x))) #else #define APR_NONNULL(x) #endif for every attribute? Would these macros be a new API that needs to wait for 1.5.x? Cheers, Stefan
Re: Adding gcc attributes to function declarations
On Sun, 15 May 2011, Stefan Fritsch wrote: I think it would be a good idea to add some more gcc attribute annotations to our header to mark parameters that must not be NULL, etc. We already have some of these for format string. In order to not break other compilers, I thought to just include #if !defined(__GNUC__) #define __attribute__(x) #endif Ignore this mail. This code is already contained in apr.h.in
Re: [Vote] Release apr-util 1.3.11
On Tuesday 26 April 2011, Rainer Jung wrote: +1 although there are still two problems on Solaris 10 for test_reslist, but not a regression. I built and made check on the following platforms: - Solaris 8 + 10, Sparc - SuSE Linux Enterprise 10 32 and 64 Bit - RedHat Enterprise Linux 5, 64 Bit Using all combinations of: apr 1.3.12 / 1.4.2 expat builtin / 2.0.1 dso disable / enable Berkeley DB 4.8.30 5.0.26 5.1.19 sqlite 3.7.2 mysql 6.0.2 (only Solaris) oracle 10.2.0.4.0 (only Solaris) All builds suceeded, all make check ran fine, except for two cases on Solaris 10 (this time not Niagara, but instead old sun4u - V240 with 2 CPUs). I reran the tests and couldn't reproduce the problem, so it is not deterministic. Out of 48 build combinations on Solaris 10, only three had a problem. This is similar to 1.3.10, but it is not always the same combinations. Like for 1.3.10 problem happens on Solaris 10 but not on Solaris 8. Details on Solaris 10 test failures - only in testreslist - two types of failures: - twice crashes (segmentation fault) - once non-terminating loop - Crashes seem not really related to used apr version (one for 1.3 and one for 1.4) I also get undeterministic test failures on the Debian build machines, mostly hangs in testreslist. It happens on mipsel and sparc much more often than on the other architectures, and some architectures had no failure at all. Which compiler are you using? If you are using gcc, it could be a gcc bug.
Re: Discuss; reset to apr-util 1.5.0-dev?
On Friday 15 April 2011, William A. Rowe Jr. wrote: On 4/15/2011 6:36 AM, Jeff Trawick wrote: On Thu, Apr 14, 2011 at 9:51 PM, William A. Rowe Jr. wr...@rowe-clan.net wrote: In order to disambiguate what was released by external entities from what the ASF APR Project has voted upon and released, are you referring to some apr-util-1.4.0 RPM from opensuse.org, or something else? archive.a.o/dist/httpd/httpd-2.3.4-alpha-deps.tar.gz; httpd-2.3.4-alpha/srclib/apr-util/include/apr_crypto.h to be very specific. [X] Stay at apr-util 1.4.0 for the next pre-2.0 release [ ] Bump to apr-util 1.4.1 for the next pre-2.0 release [ ] Bump to apr-util 1.5.0 for the next pre-2.0 release I really don't think we need to care about compatibility in/with alpha tarballs. Before we ship 2.0, we have to introduce some mechanism to preview alpha/beta features, if we don't want to continue this stagnation through the 2.0 lifespan. Odds/evens versioning might be the solution, but we should come up with some consensus before 2.0.0 is blessed. That sounds like a good idea.
Re: [Vote] Release apr-util 1.3.11
[ +1 ] Release apr-util 1.3.11 as GA Compiles and tests ok on Debian Linux unstable/x86
Re: svn commit: r1083599 - in /apr/apr/branches/1.4.x: ./ CHANGES configure.in memory/unix/apr_pools.c
On Monday 21 March 2011, Jeff Trawick wrote: This last ,\n[] causes a shell script error for me on MinGW using autoconf 2.67 and bash 3.1. Perhaps it is a system problem (no issues for me in my normal env), but the argument is not in apr trunk and I can't fathom why an empty action-if-option-not-given is useful, so I will yank. Thanks. I overlooked that fix when backporting.
APR_RING aliasing problem / segfaults with gcc 4.5
Hi, there is still the strict aliasing problem with the APR_RING macros which causes httpd to segfault with gcc 4.5 [1,2]. Has anyone found a better solution to that than marking the ring head elements volatile? A similar change has already been made for the ring entry elements in r566349. This is something we need to fix in the next 1.4.x release. If there is no other suggestion, I will commit this fix [3]. Cheers, Stefan [1] https://issues.apache.org/bugzilla/show_bug.cgi?id=50190 [2] https://issues.apache.org/bugzilla/show_bug.cgi?id=50798 [3] http://svn.mandriva.com/cgi- bin/viewvc.cgi/packages/cooker/apr/current/SOURCES/apr-1.4.2- alias.patch?view=markuppathrev=604557
Re: svn commit: r1072937 - /apr/apr/trunk/include/apr_hooks.h
On Monday 21 February 2011, traw...@apache.org wrote: Author: trawick Date: Mon Feb 21 12:07:33 2011 New Revision: 1072937 URL: http://svn.apache.org/viewvc?rev=1072937view=rev Log: add hook function args to the hook probe invocations Modified: apr/apr/trunk/include/apr_hooks.h This breaks compiling of testhooks.c: testhooks.c:51:1: error: macro APR_HOOK_PROBE_ENTRY passed 4 arguments, but takes just 3 testhooks.c: In function 'test_run_toyhook': testhooks.c:51: error: 'APR_HOOK_PROBE_ENTRY' undeclared (first use in this function) testhooks.c:51: error: (Each undeclared identifier is reported only once testhooks.c:51: error: for each function it appears in.) testhooks.c:51:1: error: macro APR_HOOK_PROBE_INVOKE passed 5 arguments, but takes just 4 testhooks.c:51: error: 'APR_HOOK_PROBE_INVOKE' undeclared (first use in this function) testhooks.c:51:1: error: macro APR_HOOK_PROBE_COMPLETE passed 6 arguments, but takes just 5 testhooks.c:51: error: 'APR_HOOK_PROBE_COMPLETE' undeclared (first use in this function) testhooks.c:51:1: error: macro APR_HOOK_PROBE_RETURN passed 5 arguments, but takes just 4 testhooks.c:51: error: 'APR_HOOK_PROBE_RETURN' undeclared (first use in this function)
Re: [PATCH] apr_file_flush_locked and short writes
On Monday 21 February 2011, Bert Huijben wrote: Beyond this, there's no a mechanism to report that all the buffered data didn't get into the file. Perhaps apr_file_flush() should loop until the entire buffer is written or it gets a non-EINTR error? Commited the looping solution as r1073142, r1073145, r1073146
Re: reducing memory fragmentation
On Sunday 20 February 2011, Jim Jagielski wrote:
On Feb 19, 2011, at 2:57 PM, Stefan Fritsch wrote:
I am sure that apr_allocator with mmap is not an advantage on all
platforms. Actually, apr allocating only multiples of 4k should
make it easier for malloc to avoid fragmentation. But glibc's
malloc fails miserably in that regard.
Is there a way during configuration time that we can do
some rough testing on best multiple to use...?
I don't know about the best multiple. One would probably have to do
benchmarks for that. But it is possible to check for mallocs that
behave similar to glibc's malloc. If several allocated chunks are not
allocated with distances that are a multiples of the page size, the
malloc is inefficient.
But before activating such a configure check, I would wait for some
feedback from some people using the mmap option in real-world.
For glibc/i386, the attached test program gives:
$ ./malloc_test ; echo $?
pagesize: 4096
minalloc: 8192
malloc offset: 8
1
If you force glibc to use mmap, the result is different. But i don't
know how efficient glibc is in this mode:
$ MALLOC_MMAP_THRESHOLD_=8192 ./malloc_test ; echo $?
pagesize: 4096
minalloc: 8192
malloc offset: 0
0
On FreeBSD amd64:
$ ./malloc_test ; echo $?
pagesize: 4096
minalloc: 8192
malloc offset: 0
0
On non-mainstream hardware (Linux glibc/ia64)
$ ./malloc_test ; echo $?
pagesize: 16384
minalloc: 16384
malloc offset: 16
1
On Linux i386 with tcmalloc:
$ LD_PRELOAD=/usr/lib/libtcmalloc_minimal.so.0.0.0 ./malloc_test ;
echo $?
pagesize: 4096
minalloc: 8192
malloc offset: 0
0
But AFAIK tcmalloc is not a good general choice either, because it
will never give memory back to the OS.
#include stdlib.h
#include stdio.h
#include unistd.h
#define ABS(i) ((i) = 0 ? (i) : (-(i)))
#define NUM 10
int main(int ac, char **av)
{
int minalloc = 8192;
char *p[NUM];
int pagesize = sysconf(_SC_PAGE_SIZE);
int offset;
int i, d;
if (pagesize minalloc)
minalloc = pagesize;
printf(pagesize: %i\n, pagesize);
printf(minalloc: %i\n, minalloc);
for (i = 0; i sizeof(NUM); i++)
p[i] = malloc(minalloc);
for (i = 1; i sizeof(NUM); i++) {
d = ABS(p[i] - p[i-1]) % pagesize;
if (d offset)
offset = d;
}
printf(malloc offset: %i\n, offset);
return offset == 0 ? 0 : 1;
}
Re: reducing memory fragmentation
On Saturday 19 February 2011, Greg Stein wrote: On Fri, Feb 18, 2011 at 16:55, Stefan Fritsch s...@sfritsch.de wrote: On Thursday 17 February 2011, Jim Jagielski wrote: Please also look at Greg's pocore memory allocation stuff... his hash stuff is also quite nice. Would be useful to be able to use that as well That looks like a much bigger change than what I have just commited. But I agree that before trying to optimize apr's allocator, one should try pocore's. Have you thought about how to do this? Probably every apr pool would be a wrapper for a pocore pool. But what about the other users of apr_allocator, like bucket allocators? There is a branch in apr for wrapping pocore's pools and hash tables[1]. Nice, I didn't know about that. Obviously, the indirection slows it down, but it does demonstrate how it would work. (and it does: I've run the entire svn test suite using this wrapping) Have you made any measurements how much the slow down is? My experiments show that mmap/munmap are NOT speed-advantageous on MacOS. But if you're looking at long-term memory usage and avoiding fragmentation... I don't have a good way to test that. That said, pocore should not be subject to fragmentation like apr. Its coalescing feature (designed w/ the APIs present, but not yet coded) will avoid much fragmentation. I am sure that apr_allocator with mmap is not an advantage on all platforms. Actually, apr allocating only multiples of 4k should make it easier for malloc to avoid fragmentation. But glibc's malloc fails miserably in that regard. For the purpose of httpd giving unused memory back to the OS, your current apr/pocore branch won't be an improvement because the bucket allocators still use apr_allocator, which will hold on to some free memory.
Re: reducing memory fragmentation
On Thursday 17 February 2011, Jim Jagielski wrote: Please also look at Greg's pocore memory allocation stuff... his hash stuff is also quite nice. Would be useful to be able to use that as well That looks like a much bigger change than what I have just commited. But I agree that before trying to optimize apr's allocator, one should try pocore's. Have you thought about how to do this? Probably every apr pool would be a wrapper for a pocore pool. But what about the other users of apr_allocator, like bucket allocators?
reducing memory fragmentation
On Friday 04 February 2011, William A. Rowe Jr. wrote: My thought is that this might not be the only thing to 'refresh' when you have an app which needs current state. It would be a chance to flush out all caches and set the state basically to apr_initialize(), modulo any open resources or currently allocated pools. It could even go so far as to free() all the unused pool memory, giving us a really fresh start on heap pages that have been fragmented to heck. ++1 for some way of handling the fragmentation... of course, we could just switch to pocore for the memory stuff in APR. :) Which helps for apr_allocator but not for the internal fragmentation within individual pools. An optional pocore feature for APR 2 sounds great, patches welcome :) The pools give their memory back to the allocator when they are cleared. Therefore at least the transaction pools and their children should not be a major problem for fragmentation. However, one major problem are the allocators not giving back memory, or if configured with max_mem_free, giving it back in random order so that it is very unlikely that the malloc implementation can give memory back to the OS. A second problem I have noticed with glibc: Its malloc allocates APR's typical multiple-of-4k chunks in a normal linked list. This means that there is some N bytes header allocated before each chunk. Now if APR has allocated say 16k, and frees it again, the hole is 16k+N. However this is not big enough to allocate two 8k chunks, which would need (8k+N)*2 == 16k+2N bytes. I think this may contribute to the fragmentation seen under Linux. In addition to that, glibc's malloc allocates smaller chunks in the same linked list, so in practice we have something like this: 8K pool chunk 8K pool chunk some bytes from a third party lib 12K pool chunk some more bytes from normal malloc 8K pool chunk This doesn't help with fragmentation either. Therefore I propose to use mmap/mumap in the apr_allocator instead of malloc, either as a compile time option or whenever max_mem_free is set for the allocator. This would then completely separate the allocations from apr_allocator and normal malloc and reduce fragmentation. And it would make apr_allocator_max_free_set actually give memory back to the OS. This would be a relatively simple change (compared to replacing the whole allocator). Sounds ok?
Re: [VOTE] Release apr-util-0.9.19 as GA
On Tuesday 12 October 2010, Jeff Trawick wrote: [+1] Release apr-util 0.9.19 as GA Tested on Debian unstable/x86 with builtin expat
Re: [vote] Release apr-util 1.3.10
On Fri, 1 Oct 2010, Jeff Trawick wrote: Tarballs are at http://apr.apache.org/dev/dist/. Windows packages are not yet available. Due to the inclusion of a fix for a potential DOS that could affect some library consumers, I hope to get enough feedback within 24 hours to release. +/-1 [+1] Release apr-util 1.3.10 as GA Compiles fine under Debian stable, tests pass. I have also verified that the DoS fix works as expected.
Re: scan-build on apr/trunk
On Saturday 31 July 2010, Philip M. Gollucci wrote: http://p6m7g8.net/~pgollucci/sb/ I might take a stab at some of these next week. $ ./buildconf $ scan-build ./configure $ scan-build make -j12 One more issue found by gcc: Error handling in apr_memcache.c is broken: apr/memcache/apr_memcache.c:793: warning: comparison of unsigned expression 0 is always false apr/memcache/apr_memcache.c:1368: warning: comparison of unsigned expression = 0 is always true
Re: svn commit: r979891 - in /apr/apr/trunk: build/ dbd/ dbm/ file_io/netware/ file_io/os2/ file_io/unix/ file_io/win32/ memory/unix/ threadproc/beos/ threadproc/unix/
On Wed, 28 Jul 2010, Ruediger Pluem wrote: On 07/28/2010 12:09 AM, s...@apache.org wrote: Author: sf Date: Tue Jul 27 22:09:45 2010 New Revision: 979891 Modified: apr/apr/trunk/dbm/apr_dbm_sdbm.c URL: http://svn.apache.org/viewvc/apr/apr/trunk/dbm/apr_dbm_sdbm.c?rev=979891r1=979890r2=979891view=diff == --- apr/apr/trunk/dbm/apr_dbm_sdbm.c (original) +++ apr/apr/trunk/dbm/apr_dbm_sdbm.c Tue Jul 27 22:09:45 2010 @@ -193,7 +193,7 @@ static apr_status_t vt_sdbm_nextkey(apr_ pkey-dsize = rd.dsize; /* store any error info into DBM, and return a status code. */ -return set_error(dbm, APR_SUCCESS); +return set_error(dbm, rv); This breaks the testdbm test of apr. Sorry, reverted in r980197 for now. This discards any error from getnext() in sdbm.c, including those from the chkpage consistency check. But ATM, I am not sure how to fix this correctly.
Re: Change FILE/... buckets to close file descriptor on destruction?
On Monday 14 June 2010, Stefan Fritsch wrote: [ ] change FILE/PIPE/SOCKET bucket in APR to close file descriptor when the last referencing bucket is destroyed [ ] same as above, but only if a new flag is set (like flag for mmap) [ ] don't change APR, work around problem in httpd For the record: Because of the compatibility headaches this change would cause in APR, I have fixed the problem in httpd instead.
Re: Change FILE/... buckets to close file descriptor on destruction?
On Tuesday 18 May 2010, Stefan Fritsch wrote: On Tuesday 18 May 2010, Joe Orton wrote: On Tue, May 18, 2010 at 09:18:23AM +0200, Stefan Fritsch wrote: On Tue, 18 May 2010, Ruediger Pluem wrote: So if you want to close this fd you IMHO would need to do some refcounting and only close it if no other filebucket still references it. The filebuckets already do refcounting. apr_bucket_shared_destroy(f) in the patch above is only true if the refcount has reached zero. They refcount the number of times the FILE bucket has been split/copied, they don't refcount the number of times the underlying apr_file_t object is used. APR does not seem to be consistent about of whether ownership of the object is passed on when you create a bucket wrapper for that object type; there are no API guarantees or constraints here. From a quick review, PIPE, MMAP, HEAP (kind of) do take ownership, FILE and SOCKET do not. Have you run the perl-framework test suite to see whether this breaks anything? This change does look like it'll break the APR test suite but only because of the way I happened to write one of the buckets tests. It does not cause any breakage in the perl-framework. As you suspected, it does break apr-util's testbuckets test. I will look if I can run the subversion test suite, too. If changing the current behaviour is considered too disruptive, one could also introduce a flag for the cleanup behaviour of FILE buckets, like there is a flag for mmap. IMHO doing the closing in the bucket cleanup is far preferable to doing it anywhere else in the core output filter. From other mail: I found no breakage in subversion 1.6.11's test suite. To get this item from the httpd 2.4 blockers list, we could vote: [ ] change FILE/PIPE/SOCKET bucket in APR to close file descriptor when the last referencing bucket is destroyed [ ] same as above, but only if a new flag is set (like flag for mmap) [ ] don't change APR, work around problem in httpd But I am not sure what introducing a new element in struct apr_bucket_file would mean for ABI compatibility. On the one hand, apr_bucket_file is public, on the other hand, there are functions like apr_bucket_file_create and apr_bucket_file_enable_mmap. Would adding an element at the end be ok for apr-util 1.4, or would that only be possible in 2.0? Cheers, Stefan
Re: File descriptor leak with mpm-event / apr file bucket cleanup
On Tue, 18 May 2010, Ruediger Pluem wrote:
--- buckets/apr_buckets_file.c.dist +0200
+++ buckets/apr_buckets_file.c
@@ -34,8 +34,7 @@
apr_bucket_file *f = data;
if (apr_bucket_shared_destroy(f)) {
-/* no need to close the file here; it will get
- * done automatically when the pool gets cleaned up */
+apr_file_close(f-fd);
apr_bucket_free(f);
}
}
I guess this would be wrong. Think of the following scenario.
1. A brigade with a file bucket.
2. The file bucket is split into two file buckets during processing in the
filter chain.
3. The first bucket is processed and destroyed. This would leave the second
file bucket with an closed file descriptor.
So if you want to close this fd you IMHO would need to do some refcounting
and only close it if no other filebucket still references it.
The filebuckets already do refcounting. apr_bucket_shared_destroy(f) in
the patch above is only true if the refcount has reached zero.
Re: File descriptor leak with mpm-event / apr file bucket cleanup
On Tuesday 18 May 2010, Joe Orton wrote: On Tue, May 18, 2010 at 09:18:23AM +0200, Stefan Fritsch wrote: On Tue, 18 May 2010, Ruediger Pluem wrote: So if you want to close this fd you IMHO would need to do some refcounting and only close it if no other filebucket still references it. The filebuckets already do refcounting. apr_bucket_shared_destroy(f) in the patch above is only true if the refcount has reached zero. They refcount the number of times the FILE bucket has been split/copied, they don't refcount the number of times the underlying apr_file_t object is used. APR does not seem to be consistent about of whether ownership of the object is passed on when you create a bucket wrapper for that object type; there are no API guarantees or constraints here. From a quick review, PIPE, MMAP, HEAP (kind of) do take ownership, FILE and SOCKET do not. Have you run the perl-framework test suite to see whether this breaks anything? This change does look like it'll break the APR test suite but only because of the way I happened to write one of the buckets tests. It does not cause any breakage in the perl-framework. As you suspected, it does break apr-util's testbuckets test. I will look if I can run the subversion test suite, too. If changing the current behaviour is considered too disruptive, one could also introduce a flag for the cleanup behaviour of FILE buckets, like there is a flag for mmap. IMHO doing the closing in the bucket cleanup is far preferable to doing it anywhere else in the core output filter. Cheers, Stefan
Re: File descriptor leak with mpm-event / apr file bucket cleanup
On Tuesday 18 May 2010, Stefan Fritsch wrote: It does not cause any breakage in the perl-framework. As you suspected, it does break apr-util's testbuckets test. I will look if I can run the subversion test suite, too. I found no breakage in subversion 1.6.11's test suite. Cheers, Stefan
cppcheck fixes
Hi,
I have tried the cppcheck static code analyzer on apr trunk and it
found some errors that may be worth fixing. For some, I have attached
a patch. These two I think are false positives/intentional:
[./dso/win32/dso.c:133]: (error) Uninitialized variable: rv
[./misc/unix/otherchild.c:85]: (error) Possible null pointer
dereference: cur
I haven't looked at the rest:
[./build/aplibtool.c:379]: (error) Memory leak: newarg
[./build/aplibtool.c:637]: (error) Resource leak: dir
[./build/jlibtool.c:978]: (error) Memory leak: path
[./build/jlibtool.c:1775]: (error) Memory leak: cctemp
[./build/jlibtool.c:1961]: (error) Resource leak: dir
[./build/jlibtool.c:1653]: (error) Data is allocated but not
initialized: newpath
[./test/testbuckets.c:108]: (error) Undefined behaviour: msg is used
wrong in call to sprintf or snprintf. Quote: If copying takes place
between objects that overlap as a result of a call to sprintf() or
snprintf(), the results are undefined.
Given the relatively low rate of false positives, I think cppcheck is
a nice tool.
Cheers,
Stefan
Index: memory/unix/apr_pools.c
===
--- memory/unix/apr_pools.c (Revision 930965)
+++ memory/unix/apr_pools.c (Arbeitskopie)
@@ -1427,6 +1427,7 @@
node = pool-nodes;
if (node == NULL || node-index == 64) {
if ((node = malloc(SIZEOF_DEBUG_NODE_T)) == NULL) {
+free(mem);
if (pool-abort_fn)
pool-abort_fn(APR_ENOMEM);
Index: dbd/apr_dbd_oracle.c
===
--- dbd/apr_dbd_oracle.c (Revision 930965)
+++ dbd/apr_dbd_oracle.c (Arbeitskopie)
@@ -1758,8 +1758,8 @@
{
int ret = 1; /* no transaction is an error cond */
sword status;
-apr_dbd_t *handle = trans-handle;
if (trans) {
+apr_dbd_t *handle = trans-handle;
switch (trans-status) {
case TRANS_NONE: /* No trans is an error here */
status = OCI_ERROR;
Index: threadproc/beos/proc.c
===
--- threadproc/beos/proc.c (Revision 930965)
+++ threadproc/beos/proc.c (Arbeitskopie)
@@ -362,8 +362,9 @@
== APR_SUCCESS)
rv = apr_file_inherit_set(attr-child_in);
}
+}
-if (parent_in != NULL rv == APR_SUCCESS) {
+if (parent_in != NULL rv == APR_SUCCESS)
rv = apr_file_dup(attr-parent_in, parent_in, attr-pool);
return rv;
@@ -391,7 +392,7 @@
}
}
-if (parent_out != NULL rv == APR_SUCCESS) {
+if (parent_out != NULL rv == APR_SUCCESS)
rv = apr_file_dup(attr-parent_out, parent_out, attr-pool);
return rv;
@@ -419,7 +420,7 @@
}
}
-if (parent_err != NULL rv == APR_SUCCESS) {
+if (parent_err != NULL rv == APR_SUCCESS)
rv = apr_file_dup(attr-parent_err, parent_err, attr-pool);
return rv;
Index: file_io/unix/open.c
===
--- file_io/unix/open.c (Revision 930965)
+++ file_io/unix/open.c (Arbeitskopie)
@@ -180,13 +180,17 @@
if (!(flag APR_FOPEN_NOCLEANUP)) {
int flags;
-if ((flags = fcntl(fd, F_GETFD)) == -1)
+if ((flags = fcntl(fd, F_GETFD)) == -1) {
+close(fd);
return errno;
+}
if ((flags FD_CLOEXEC) == 0) {
flags |= FD_CLOEXEC;
-if (fcntl(fd, F_SETFD, flags) == -1)
+if (fcntl(fd, F_SETFD, flags) == -1) {
+close(fd);
return errno;
+}
}
}
Re: Finding memory leaks in httpd and httpd modules
On Wednesday 17 February 2010, Joe Orton wrote: On Wed, Feb 17, 2010 at 09:12:03AM -0500, Jeff Trawick wrote: a. get the server to steady state ... b. see what causes the heap to expand (brk/sbrk) This is what I do too, FWIW. It's primitive but usually effective. What about adding some code to apr-util's bucket debugging that logs when destroyed brigades (i.e. where the pool cleanup has been removed) are reused? I think this may be a common cause for memory leaks. A simpler alternative would be to set bb-pool and bb-bucket_alloc to NULL in apr_brigade_destroy(). The latter could maybe even be enabled in some apr-util maintainer mode (just like httpd enables some debugging in maintainer mode).
Finding memory leaks in brigade usage
On Wednesday 17 February 2010, Jeff Trawick wrote:
What about adding some code to apr-util's bucket debugging that
logs when destroyed brigades (i.e. where the pool cleanup has
been removed) are reused? I think this may be a common cause for
memory leaks.
as in something like this?
apr_brigade_destroy(foo-bb)
...
if (!foo-bb) {
foo-bb = apr_brigade_create();
}
...
APR_BRIGADE_INSERT_TAIL(foo-bb, b);
/* no automatic cleanup of b */
Exactly.
A simpler alternative would be to set bb-pool and
bb-bucket_alloc to NULL in apr_brigade_destroy(). The latter
could maybe even be enabled in some apr-util maintainer mode
(just like httpd enables some debugging in maintainer mode).
APR_BRIGADE_CHECK_CONSISTENCY() would presumably be changed to
abort if !bb-pool
True, but that would only be triggered if brigade debugging is
enabled. I was hoping to also get something more light-weight. But I
now see that setting pool and bucket_alloc to NULL won't trigger
segfaults when buckets are just moved from one brigade to another.
Maybe it's not worth the effort without full APR_BUCKET_DEBUG.
Re: [VOTE] release apr-1.4.2?
On Tuesday 26 January 2010, William A. Rowe Jr. wrote: Pushing to the mirrors now. It seems to be on the mirrors by now. Time to update the website?
Re: Quick things, apr/apr-util releases
On Tuesday 15 December 2009, William A. Rowe Jr. wrote: Not quite; the scenario that follows is; - user builds/installs apr-util-1.4.1 w/ crypto - user builds/installs crypto-using application The probability that this happens before the user installs httpd-2.3.x-alpha with x 4 and an included, released apr 1.4.y is _very_ close to zero. I don't think it makes sense to spend developer time for this corner case. But I am not an apr committer, of course. Cheers, Stefan
Re: DBD mysql driver segfault on cleanup when used together with legacy modules in apache using mysql
On Wednesday 28 October 2009, Viktor Griph wrote: There is a possiblility that the mysql library has been unloaded by the DSO loader before the cleanup function that tries to call mysql_thread_end() is called, casusing a segmentation fault. In my particular usecase I've been trying to use a module using a dbd backend for mysql together with the module mod_auth_mysql. The mysql library is dynamically linked when the mod_auth_mysql is loaded, and is unlikned before the cleanup in mod_dbd_mysql is called, leaving a call to an unloaded library. Not very likely in this case, but it may make sense to check it: You do use libmysqlclient_r.so for both mod_dbd_mysql and mod_auth_mysql? If you use libmysqlclient_r.so for one and libmysqlclient.so for the other, you will get strange segfaults.
Re: DBD mysql driver segfault on cleanup when used together with legacy modules in apache using mysql
On Wednesday 28 October 2009, Viktor Griph wrote: Not very likely in this case, but it may make sense to check it: You do use libmysqlclient_r.so for both mod_dbd_mysql and mod_auth_mysql? If you use libmysqlclient_r.so for one and libmysqlclient.so for the other, you will get strange segfaults. Actually that was the case. Gentoo links mod_auth_mysql to libmysqlclient, and apr-util with libmysqlclient_r. Manually rebuilding mod_auth_mysql linked to libmysqlclient_r solves the problem. Thanks. I'll file a bug to Gentoo. Ok. This is what happened: start: - linker loads mod_auth_mysql - linker loads libmysqlclient - linker resolves symbols in mod_auth_mysql against libmysqlclient - linker loads mod_dbd_mysql/libaprutil1-dbd-mysql - linker loads libmysqlclient_r - linker resolves symbols in mod_dbd_mysql/libaprutil1-dbd-mysql against libmysqlclient(!) unload: - linker unloads mod_auth_mysql and libmysqlclient - mod_dbd_mysql crashes This is this mysql bug: http://bugs.mysql.com/bug.php?id=32196 It would be nice if the Gentoo maintainer could add his voice there to make the mysql people fix it. Cheers, Stefan
Re: [vote] release apr 1.3.9?
Votes open for 48 hours or until we register sufficient +1's for release. Compiles ok with SunCC on Solaris9/Sparc and Solaris10/X86. I get one IPv6 related test failure in testsockets, but that is not a regression (happens with 1.3.8, too) and could be due to the way my test machines are set up. Cheers, Stefan
Workaround for linux/hppa bug
Hi,
there is a long standing bug in the Linux/hppa kernel that flock()
returns EAGAIN and not EWOULDBLOCK. On all other architectures, these
two are defined to be the same. This causes testprocmutex to fail.
Cheers,
Stefan
--- apr-1.3.3~/locks/unix/proc_mutex.c
+++ apr-1.3.3/locks/unix/proc_mutex.c
@@ -683,7 +683,7 @@
rc = flock(mutex-interproc-filedes, LOCK_EX | LOCK_NB);
} while (rc 0 errno == EINTR);
if (rc 0) {
-if (errno == EWOULDBLOCK) {
+if (errno == EWOULDBLOCK || errno == EAGAIN) {
return APR_EBUSY;
}
return errno;
apr_file_dup and FD_CLOEXEC revisited
Hi,
I finally got around to test the stderr reopening behaviour with the
various FD_CLOEXEC bits in trunk. I noticed that this still does not
work, i.e. when mod_php exec()s another program, stderr gets closed.
The problem is in apr_file_dup2(). It should retain the
INHERIT/NOCLEANUP flags of new_file, but currently it checks those
flags in old_file. There was some discussion about this starting at
http://www.mail-archive.com/dev@apr.apache.org/msg21357.html
The attached patch fixes the behaviour. With it, a program exec()ed by
mod_php has exactly stdin, stdout, and stderr open, and nothing else.
With the patch, I think the FD_CLOEXEC bits can be backported to 1.4.
The relevant commits are
747990
748361
748371
748565
748988
749810
One remaining problem is that when apr is compiled with these changes
on a recent linux kernel, it won't run on older kernels due to the
various new syscalls missing. Therefore, maybe it is not a good idea
to backport this to 1.3. Or one could disable the use of the new
syscalls for 1.3.
Cheers,
Stefan
diff --git a/file_io/unix/filedup.c b/file_io/unix/filedup.c
index d40004f..47ea4f3 100644
--- a/file_io/unix/filedup.c
+++ b/file_io/unix/filedup.c
@@ -35,12 +35,12 @@ static apr_status_t file_dup(apr_file_t **new_file,
return APR_EINVAL;
}
#ifdef HAVE_DUP3
-if (!(old_file-flags (APR_FILE_NOCLEANUP|APR_INHERIT)))
+if (!((*new_file)-flags (APR_FILE_NOCLEANUP|APR_INHERIT)))
flags |= O_CLOEXEC;
rv = dup3(old_file-filedes, (*new_file)-filedes, flags);
#else
rv = dup2(old_file-filedes, (*new_file)-filedes);
-if (!(old_file-flags (APR_FILE_NOCLEANUP|APR_INHERIT))) {
+if (!((*new_file)-flags (APR_FILE_NOCLEANUP|APR_INHERIT))) {
int flags;
if (rv == -1)
