Re: [PATCH] WIN64: apr_pools.c
Jeff Trawick wrote: APR_32BIT_TRUNC_CAST defined only in private headers only so that a) we don't add yet more symbols to API b) we continually identify where our API is broken w.r.t. handling quantities properly in a 64-bit world I believe this addresses the issues Allan - Index: srclib/apr/include/apr.hnw === RCS file: /home/cvs/apr/include/apr.hnw,v retrieving revision 1.49 diff -U3 -r1.49 apr.hnw --- srclib/apr/include/apr.hnw 1 Oct 2004 19:24:03 - 1.49 +++ srclib/apr/include/apr.hnw 6 Oct 2004 18:48:51 - @@ -330,7 +330,6 @@ #define APR_UINT64_T_HEX_FMT llx #define APR_TIME_T_FMT APR_INT64_T_FMT -#define APR_DWORD_MAX 0xUL/* 2^32*/ /** @} */ #ifdef __cplusplus Index: srclib/apr/include/apr.hw === RCS file: /home/cvs/apr/include/apr.hw,v retrieving revision 1.127 diff -U3 -r1.127 apr.hw --- srclib/apr/include/apr.hw 1 Oct 2004 19:24:03 - 1.127 +++ srclib/apr/include/apr.hw 6 Oct 2004 18:48:51 - @@ -342,8 +342,6 @@ #define APR_SIZEOF_VOIDP 4 #endif -#define APR_DWORD_MAX 0xUL - /* XXX These simply don't belong here, perhaps in apr_portable.h * based on some APR_HAVE_PID/GID/UID? */ Index: srclib/apr/include/arch/apr_private_common.h === RCS file: /home/cvs/apr/include/arch/apr_private_common.h,v retrieving revision 1.2 diff -U3 -r1.2 apr_private_common.h --- srclib/apr/include/arch/apr_private_common.h13 Feb 2004 09:38:29 - 1.2 +++ srclib/apr/include/arch/apr_private_common.h6 Oct 2004 18:48:51 - @@ -33,4 +33,9 @@ char separator, apr_pool_t *p); +/* temporary defines to handle 64bit compile mismatches */ +#define APR_INT_TRUNC_CASTint +#define APR_UINT32_TRUNC_CAST apr_uint32_t +#define APR_UINT32_MAX0xUL + #endif /*APR_PRIVATE_COMMON_H*/ Index: srclib/apr/include/arch/netware/apr_private.h === RCS file: /home/cvs/apr/include/arch/netware/apr_private.h,v retrieving revision 1.25 diff -U3 -r1.25 apr_private.h --- srclib/apr/include/arch/netware/apr_private.h 6 Jul 2004 22:40:49 - 1.25 +++ srclib/apr/include/arch/netware/apr_private.h 6 Oct 2004 18:48:51 - @@ -172,6 +172,9 @@ #define APR_OFF_T_STRFN strtol #endif +/* used to check DWORD overflow for 64bit compiles */ +#define APR_DWORD_MAX 0xUL + /* * Include common private declarations. */ Index: srclib/apr/include/arch/win32/apr_private.h === RCS file: /home/cvs/apr/include/arch/win32/apr_private.h,v retrieving revision 1.38 diff -U3 -r1.38 apr_private.h --- srclib/apr/include/arch/win32/apr_private.h 4 Jun 2004 23:26:02 - 1.38 +++ srclib/apr/include/arch/win32/apr_private.h 6 Oct 2004 18:48:52 - @@ -158,6 +158,9 @@ #define APR_OFF_T_STRFN strtoi #endif +/* used to check for DWORD overflow in 64bit compiles */ +#define APR_DWORD_MAX 0xUL + /* * Include common private declarations. */ Index: srclib/apr/memory/unix/apr_pools.c === RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v retrieving revision 1.206 diff -U3 -r1.206 apr_pools.c --- srclib/apr/memory/unix/apr_pools.c 17 Jun 2004 14:13:58 - 1.206 +++ srclib/apr/memory/unix/apr_pools.c 6 Oct 2004 18:48:53 - @@ -139,9 +139,10 @@ } APR_DECLARE(void) apr_allocator_max_free_set(apr_allocator_t *allocator, - apr_size_t size) + apr_size_t in_size) { apr_uint32_t max_free_index; +apr_uint32_t size = (APR_UINT32_TRUNC_CAST)in_size; #if APR_HAS_THREADS apr_thread_mutex_t *mutex; @@ -168,7 +169,8 @@ apr_memnode_t *allocator_alloc(apr_allocator_t *allocator, apr_size_t size) { apr_memnode_t *node, **ref; -apr_uint32_t i, index, max_index; +apr_uint32_t max_index; +apr_size_t i, index; /* Round up the block size to the next boundary, but always * allocate at least a certain size (MIN_ALLOC). @@ -181,6 +183,10 @@ * dividing its size by the boundary size */ index = (size BOUNDARY_INDEX) - 1; + +if (index APR_UINT32_MAX) { +return NULL; +} /* First see if there are any nodes in the area we know * our node will fit into. @@ -293,7 +299,7 @@ return NULL; node-next = NULL; -node-index = index; +node-index = (APR_UINT32_TRUNC_CAST)index; node-first_avail = (char *)node + APR_MEMNODE_T_SIZE; node-endp = (char *)node + size; @@ -582,7 +588,7 @@ { apr_memnode_t *active, *node; void
Re: [PATCH] WIN64: apr_pools.c
I'm like Jeff, I won't lose any sleep if we just say that 64 bit APR 1.0 cannot handle very large memory objects and go the path of validating restricting and casting. At least let's pursue that path and see how ugly it gets. Going with plan 'B'... I tend to think it would be useful to use a #define for any 64bit warning-quieting casts so that they can be easily identified ripped out for APR 2.0 - comments? Allan Index: srclib/apr/include/apr.h.in === RCS file: /home/cvs/apr/include/apr.h.in,v retrieving revision 1.137 diff -U3 -r1.137 apr.h.in --- srclib/apr/include/apr.h.in 5 Jun 2004 11:52:43 - 1.137 +++ srclib/apr/include/apr.h.in 5 Oct 2004 17:56:20 - @@ -266,6 +266,10 @@ /* Are we big endian? */ #define APR_IS_BIGENDIAN @bigendian@ +#define APR_INT_CASTint +#define APR_UINT32_CAST apr_uint32_t +#define APR_UINT32_MAX 0xUL + /* Mechanisms to properly type numeric literals */ @int64_literal@ @uint64_literal@ Index: srclib/apr/include/apr.hnw === RCS file: /home/cvs/apr/include/apr.hnw,v retrieving revision 1.49 diff -U3 -r1.49 apr.hnw --- srclib/apr/include/apr.hnw 1 Oct 2004 19:24:03 - 1.49 +++ srclib/apr/include/apr.hnw 5 Oct 2004 17:56:20 - @@ -330,6 +330,9 @@ #define APR_UINT64_T_HEX_FMT llx #define APR_TIME_T_FMT APR_INT64_T_FMT +#define APR_INT_CASTint +#define APR_UINT32_CAST apr_uint32_t +#define APR_UINT32_MAX 0xUL #define APR_DWORD_MAX 0xUL/* 2^32*/ /** @} */ Index: srclib/apr/include/apr.hw === RCS file: /home/cvs/apr/include/apr.hw,v retrieving revision 1.127 diff -U3 -r1.127 apr.hw --- srclib/apr/include/apr.hw 1 Oct 2004 19:24:03 - 1.127 +++ srclib/apr/include/apr.hw 5 Oct 2004 17:56:20 - @@ -342,6 +342,9 @@ #define APR_SIZEOF_VOIDP 4 #endif +#define APR_INT_CASTint +#define APR_UINT32_CAST apr_uint32_t +#define APR_UINT32_MAX 0xUL #define APR_DWORD_MAX 0xUL /* XXX These simply don't belong here, perhaps in apr_portable.h Index: srclib/apr/memory/unix/apr_pools.c === RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v retrieving revision 1.206 diff -U3 -r1.206 apr_pools.c --- srclib/apr/memory/unix/apr_pools.c 17 Jun 2004 14:13:58 - 1.206 +++ srclib/apr/memory/unix/apr_pools.c 5 Oct 2004 17:56:20 - @@ -139,9 +139,10 @@ } APR_DECLARE(void) apr_allocator_max_free_set(apr_allocator_t *allocator, - apr_size_t size) + apr_size_t in_size) { apr_uint32_t max_free_index; +apr_uint32_t size = (APR_UINT32_CAST)in_size; #if APR_HAS_THREADS apr_thread_mutex_t *mutex; @@ -168,7 +169,8 @@ apr_memnode_t *allocator_alloc(apr_allocator_t *allocator, apr_size_t size) { apr_memnode_t *node, **ref; -apr_uint32_t i, index, max_index; +apr_uint32_t max_index; +apr_size_t i, index; /* Round up the block size to the next boundary, but always * allocate at least a certain size (MIN_ALLOC). @@ -181,6 +183,10 @@ * dividing its size by the boundary size */ index = (size BOUNDARY_INDEX) - 1; + +if (index APR_UINT32_MAX) { +return NULL; +} /* First see if there are any nodes in the area we know * our node will fit into. @@ -293,7 +299,7 @@ return NULL; node-next = NULL; -node-index = index; +node-index = (APR_UINT32_CAST)index; node-first_avail = (char *)node + APR_MEMNODE_T_SIZE; node-endp = (char *)node + size; @@ -582,7 +588,7 @@ { apr_memnode_t *active, *node; void *mem; -apr_uint32_t free_index; +apr_size_t free_index; size = APR_ALIGN_DEFAULT(size); active = pool-active; @@ -620,7 +626,7 @@ free_index = (APR_ALIGN(active-endp - active-first_avail + 1, BOUNDARY_SIZE) - BOUNDARY_SIZE) BOUNDARY_INDEX; -active-free_index = free_index; +active-free_index = (APR_UINT32_CAST)free_index; node = active-next; if (free_index = node-free_index) return mem; @@ -877,7 +883,7 @@ apr_size_t cur_len, size; char *strp; apr_pool_t *pool; -apr_uint32_t free_index; +apr_size_t free_index; pool = ps-pool; active = ps-node; @@ -907,7 +913,7 @@ free_index = (APR_ALIGN(active-endp - active-first_avail + 1, BOUNDARY_SIZE) - BOUNDARY_SIZE) BOUNDARY_INDEX; -active-free_index = free_index; +active-free_index = (APR_UINT32_CAST)free_index; node = active-next; if (free_index node-free_index) { do { @@ -948,7 +954,7 @@ char *strp; apr_size_t size; apr_memnode_t *active, *node; -apr_uint32_t free_index; +
Re: [PATCH] WIN64: apr_pools.c
On Tue, 05 Oct 2004 14:02:20 -0400, Allan Edwards [EMAIL PROTECTED] wrote: I'm like Jeff, I won't lose any sleep if we just say that 64 bit APR 1.0 cannot handle very large memory objects and go the path of validating restricting and casting. At least let's pursue that path and see how ugly it gets. Going with plan 'B'... I tend to think it would be useful to use a #define for any 64bit warning-quieting casts so that they can be easily identified ripped out for APR 2.0 - comments? Allan Index: srclib/apr/include/apr.h.in I'd suggest APR_32BIT_TRUNC_CAST defined only in private headers only so that a) we don't add yet more symbols to API b) we continually identify where our API is broken w.r.t. handling quantities properly in a 64-bit world
Re: [PATCH] WIN64: apr_pools.c
On Wed, 29 Sep 2004 23:00:52 +0100, Joe Orton [EMAIL PROTECTED] wrote: On Wed, Sep 29, 2004 at 05:35:34PM -0400, Allan Edwards wrote: In order to eliminate the following warnings for Win64 builds the appended patch is needed. However this changes fields in the apr_memnode_t structure in apr/include/apr_allocator.h Seems that this strucure really should be private and moved along with struct apr_allocator_t to include/arch/unix/apr_arch_allocator.h but either moving it or changing it may be construed as changing the 1.0 API. Personally I don't think so but if anyone thinks otherwise let's discuss. It is part of the API, it can't be changed like this in 1.x. You can't use the apr_allocator_* API without using the apr_memnode_t structure, so I don't see how it could be made private either. so 64-bit APR can't handle very large memory objects (which isn't something I'll lose sleep over any time soon)... something obviously needs to be done with this type of issue to avoid the multitude of compile warnings and also fail any request to handle larger (e.g., maybe an allocation function has a parameter which can hold more than uint32, but since allocator can't store that much the request must be failed) but apr_allocator_max_free_set() has no retcode, so it has to be documented that it can't handle more than will fit in 32-bit unsigned int, and on entry it needs to copy with truncation (and no warning) the apr_size_t parameter into a local 32-bit variable
Re: [PATCH] WIN64: apr_pools.c
Joe Orton wrote: It is part of the API, it can't be changed like this in 1.x. You can't use the apr_allocator_* API without using the apr_memnode_t structure, The allocator API uses an incomplete type for apr_allocator_t and in the same way should (actually it does) use an incomplete type for apr_memnode_t. It just seems like an oversight that the definition of apr_memnode_t was left in apr_allocator.h so I don't see how it could be made private either. This patch demonstates that it can be made private, httpd builds cleanly against it. The only reason I can see that this breaks the API is if someone were doing strange and unnatural things outside of the normal use of the API. Do we want to deny this change because of that remote possibility? Allan Index: apr/include/apr_allocator.h === RCS file: /home/cvs/apr/include/apr_allocator.h,v retrieving revision 1.19 diff -U3 -r1.19 apr_allocator.h --- apr/include/apr_allocator.h 13 Feb 2004 09:38:28 - 1.19 +++ apr/include/apr_allocator.h 30 Sep 2004 15:09:58 - @@ -41,27 +41,6 @@ /** the structure which holds information about the allocation */ typedef struct apr_memnode_t apr_memnode_t; -/** basic memory node structure - * @note The next, ref and first_avail fields are available for use by the - * caller of apr_allocator_alloc(), the remaining fields are read-only. - * The next field has to be used with caution and sensibly set when the - * memnode is passed back to apr_allocator_free(). See apr_allocator_free() - * for details. - * The ref and first_avail fields will be properly restored by - * apr_allocator_free(). - */ -struct apr_memnode_t { -apr_memnode_t *next;/** next memnode */ -apr_memnode_t **ref;/** reference to self */ -apr_uint32_t index; /** size */ -apr_uint32_t free_index; /** how much free */ -char *first_avail; /** pointer to first free memory */ -char *endp;/** pointer to end of free memory */ -}; - -/** The base size of a memory node - aligned. */ -#define APR_MEMNODE_T_SIZE APR_ALIGN_DEFAULT(sizeof(apr_memnode_t)) - /** Symbolic constants */ #define APR_ALLOCATOR_MAX_FREE_UNLIMITED 0 Index: apr/memory/unix/apr_pools.c === RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v retrieving revision 1.206 diff -U3 -r1.206 apr_pools.c --- apr/memory/unix/apr_pools.c 17 Jun 2004 14:13:58 - 1.206 +++ apr/memory/unix/apr_pools.c 30 Sep 2004 15:09:58 - @@ -21,7 +21,7 @@ #include apr_strings.h #include apr_general.h #include apr_pools.h -#include apr_allocator.h +#include arch/unix/apr_arch_allocator.h #include apr_lib.h #include apr_thread_mutex.h #include apr_hash.h @@ -63,9 +63,9 @@ */ struct apr_allocator_t { -apr_uint32_tmax_index; -apr_uint32_tmax_free_index; -apr_uint32_tcurrent_free_index; +apr_size_t max_index; +apr_size_t max_free_index; +apr_size_t current_free_index; #if APR_HAS_THREADS apr_thread_mutex_t *mutex; #endif /* APR_HAS_THREADS */ @@ -141,7 +141,7 @@ APR_DECLARE(void) apr_allocator_max_free_set(apr_allocator_t *allocator, apr_size_t size) { -apr_uint32_t max_free_index; +apr_size_t max_free_index; #if APR_HAS_THREADS apr_thread_mutex_t *mutex; @@ -168,7 +168,7 @@ apr_memnode_t *allocator_alloc(apr_allocator_t *allocator, apr_size_t size) { apr_memnode_t *node, **ref; -apr_uint32_t i, index, max_index; +apr_size_t i, index, max_index; /* Round up the block size to the next boundary, but always * allocate at least a certain size (MIN_ALLOC). @@ -304,8 +304,8 @@ void allocator_free(apr_allocator_t *allocator, apr_memnode_t *node) { apr_memnode_t *next, *freelist = NULL; -apr_uint32_t index, max_index; -apr_uint32_t max_free_index, current_free_index; +apr_size_t index, max_index; +apr_size_t max_free_index, current_free_index; #if APR_HAS_THREADS if (allocator-mutex) @@ -582,7 +582,7 @@ { apr_memnode_t *active, *node; void *mem; -apr_uint32_t free_index; +apr_size_t free_index; size = APR_ALIGN_DEFAULT(size); active = pool-active; @@ -877,7 +877,7 @@ apr_size_t cur_len, size; char *strp; apr_pool_t *pool; -apr_uint32_t free_index; +apr_size_t free_index; pool = ps-pool; active = ps-node; @@ -948,7 +948,7 @@ char *strp; apr_size_t size; apr_memnode_t *active, *node; -apr_uint32_t free_index; +apr_size_t free_index; ps.node = active = pool-active; ps.pool = pool; Index: apr-util/buckets/apr_buckets_alloc.c === RCS file: /home/cvs/apr-util/buckets/apr_buckets_alloc.c,v retrieving
Re: [PATCH] WIN64: apr_pools.c
On Thu, Sep 30, 2004 at 11:22:41AM -0400, Allan Edwards wrote: so I don't see how it could be made private either. This patch demonstates that it can be made private, Only if you redefine private :) --- apr-util/buckets/apr_buckets_alloc.c 10 Aug 2004 22:00:16 - 1.16 +++ apr-util/buckets/apr_buckets_alloc.c 30 Sep 2004 15:09:59 - @@ -16,7 +16,7 @@ #include stdlib.h #include apr_buckets.h -#include apr_allocator.h +#include arch/unix/apr_arch_allocator.h #define ALLOC_AMT (8192 - APR_MEMNODE_T_SIZE)
Re: [PATCH] WIN64: apr_pools.c
At 11:48 AM 9/30/2004, Joe Orton wrote: On Thu, Sep 30, 2004 at 11:22:41AM -0400, Allan Edwards wrote: so I don't see how it could be made private either. This patch demonstates that it can be made private, Only if you redefine private :) This is all pedantic, because you can't make this interface private until APR 2.0. The only fix we could possibly adopt before then, outside of -validating- and restricting these to 32 bit quantities on those platforms with mismatches, is to -retract- the release of APR 1.0 on all platforms with 32bit int/64bit pointers. Thoughts? Bill
Re: [PATCH] WIN64: apr_pools.c
William A. Rowe, Jr. wrote: The only fix we could possibly adopt before then, outside of -validating- and restricting these to 32 bit quantities on those platforms with mismatches, is to -retract- the release of APR 1.0 on all platforms with 32bit int/64bit pointers. I'm like Jeff, I won't lose any sleep if we just say that 64 bit APR 1.0 cannot handle very large memory objects and go the path of validating restricting and casting. At least let's pursue that path and see how ugly it gets. Allan
[PATCH] WIN64: apr_pools.c
In order to eliminate the following warnings for Win64 builds the appended patch is needed. However this changes fields in the apr_memnode_t structure in apr/include/apr_allocator.h Seems that this strucure really should be private and moved along with struct apr_allocator_t to include/arch/unix/apr_arch_allocator.h but either moving it or changing it may be construed as changing the 1.0 API. Personally I don't think so but if anyone thinks otherwise let's discuss. Allan --- apr_pools.c(154) : warning C4267: '=' : conversion from 'size_t' to 'apr_uint32_t', possible loss of data apr_pools.c(183) : warning C4267: '=' : conversion from 'size_t' to 'apr_uint32_t', possible loss of data apr_pools.c(621) : warning C4244: '=' : conversion from '__int64' to 'apr_uint32_t', possible loss of data apr_pools.c(908) : warning C4244: '=' : conversion from '__int64' to 'apr_uint32_t', possible loss of data apr_pools.c(1009) : warning C4244: '=' : conversion from '__int64' to 'apr_uint32_t', possible loss of data Index: apr/include/apr_allocator.h === RCS file: /home/cvs/apr/include/apr_allocator.h,v retrieving revision 1.19 diff -U3 -r1.19 apr_allocator.h --- apr/include/apr_allocator.h 13 Feb 2004 09:38:28 - 1.19 +++ apr/include/apr_allocator.h 29 Sep 2004 20:29:37 - @@ -53,8 +53,8 @@ struct apr_memnode_t { apr_memnode_t *next;/** next memnode */ apr_memnode_t **ref;/** reference to self */ -apr_uint32_t index; /** size */ -apr_uint32_t free_index; /** how much free */ +apr_size_t index; /** size */ +apr_size_t free_index; /** how much free */ char *first_avail; /** pointer to first free memory */ char *endp;/** pointer to end of free memory */ }; Index: apr/memory/unix/apr_pools.c === RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v retrieving revision 1.206 diff -U3 -r1.206 apr_pools.c --- apr/memory/unix/apr_pools.c 17 Jun 2004 14:13:58 - 1.206 +++ apr/memory/unix/apr_pools.c 29 Sep 2004 20:29:37 - @@ -63,9 +63,9 @@ */ struct apr_allocator_t { -apr_uint32_tmax_index; -apr_uint32_tmax_free_index; -apr_uint32_tcurrent_free_index; +apr_size_t max_index; +apr_size_t max_free_index; +apr_size_t current_free_index; #if APR_HAS_THREADS apr_thread_mutex_t *mutex; #endif /* APR_HAS_THREADS */ @@ -141,7 +141,7 @@ APR_DECLARE(void) apr_allocator_max_free_set(apr_allocator_t *allocator, apr_size_t size) { -apr_uint32_t max_free_index; +apr_size_t max_free_index; #if APR_HAS_THREADS apr_thread_mutex_t *mutex; @@ -168,7 +168,7 @@ apr_memnode_t *allocator_alloc(apr_allocator_t *allocator, apr_size_t size) { apr_memnode_t *node, **ref; -apr_uint32_t i, index, max_index; +apr_size_t i, index, max_index; /* Round up the block size to the next boundary, but always * allocate at least a certain size (MIN_ALLOC). @@ -304,8 +304,8 @@ void allocator_free(apr_allocator_t *allocator, apr_memnode_t *node) { apr_memnode_t *next, *freelist = NULL; -apr_uint32_t index, max_index; -apr_uint32_t max_free_index, current_free_index; +apr_size_t index, max_index; +apr_size_t max_free_index, current_free_index; #if APR_HAS_THREADS if (allocator-mutex) @@ -582,7 +582,7 @@ { apr_memnode_t *active, *node; void *mem; -apr_uint32_t free_index; +apr_size_t free_index; size = APR_ALIGN_DEFAULT(size); active = pool-active; @@ -877,7 +877,7 @@ apr_size_t cur_len, size; char *strp; apr_pool_t *pool; -apr_uint32_t free_index; +apr_size_t free_index; pool = ps-pool; active = ps-node; @@ -948,7 +948,7 @@ char *strp; apr_size_t size; apr_memnode_t *active, *node; -apr_uint32_t free_index; +apr_size_t free_index; ps.node = active = pool-active; ps.pool = pool;
Re: [PATCH] WIN64: apr_pools.c
On Wed, Sep 29, 2004 at 05:35:34PM -0400, Allan Edwards wrote: In order to eliminate the following warnings for Win64 builds the appended patch is needed. However this changes fields in the apr_memnode_t structure in apr/include/apr_allocator.h Seems that this strucure really should be private and moved along with struct apr_allocator_t to include/arch/unix/apr_arch_allocator.h but either moving it or changing it may be construed as changing the 1.0 API. Personally I don't think so but if anyone thinks otherwise let's discuss. It is part of the API, it can't be changed like this in 1.x. You can't use the apr_allocator_* API without using the apr_memnode_t structure, so I don't see how it could be made private either. @@ -53,8 +53,8 @@ struct apr_memnode_t { apr_memnode_t *next;/** next memnode */ apr_memnode_t **ref;/** reference to self */ -apr_uint32_t index; /** size */ -apr_uint32_t free_index; /** how much free */ +apr_size_t index; /** size */ +apr_size_t free_index; /** how much free */ char *first_avail; /** pointer to first free memory */ char *endp;/** pointer to end of free memory */ };