Re: svn commit: r1372054 - in /httpd/httpd/trunk: CHANGES server/util.c

2012-08-13 Thread Daniel Gruno
On 08/12/2012 03:15 PM, Ruediger Pluem wrote:
 
 
 humbed...@apache.org wrote:
 Author: humbedooh
 Date: Sun Aug 12 07:45:55 2012
 New Revision: 1372054

 URL: http://svn.apache.org/viewvc?rev=1372054view=rev
 Log:
 core:
 Be less strict when checking whether Content-Type is set to 
 application/x-www-form-urlencoded 
 when parsing POST data, or we risk losing data with an appended charset.

 PR 53698
 Reported by: Petter Berntsen  sluggr gmail.com 

 Modified:
 httpd/httpd/trunk/CHANGES
 httpd/httpd/trunk/server/util.c

 
 Modified: httpd/httpd/trunk/server/util.c
 URL: 
 http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1372054r1=1372053r2=1372054view=diff
 ==
 --- httpd/httpd/trunk/server/util.c (original)
 +++ httpd/httpd/trunk/server/util.c Sun Aug 12 07:45:55 2012
 @@ -2406,7 +2406,7 @@ AP_DECLARE(int) ap_parse_form_data(reque
  
  /* sanity check - we only support forms for now */
  ct = apr_table_get(r-headers_in, Content-Type);
 -if (!ct || strcmp(application/x-www-form-urlencoded, ct)) {
 +if (!ct || ap_strcmp_match(ct, application/x-www-form-urlencoded*)) {
 
 ap_strcmp_match seems to be a lot of overhead for just prefix matching a 
 string.
 How about
 
  strncmp(application/x-www-form-urlencoded, ct, 33)
 
 instead.
 
 Regards
 
 Rüdiger
 
Yeah, that approach seems better. Took me a couple of tries to commit
the right version though - I blame the lack of coffee coupled with heavy
partying last night.

With regards
Daniel.


Re: svn commit: r1372054 - in /httpd/httpd/trunk: CHANGES server/util.c

2012-08-13 Thread Nick Kew

On 12 Aug 2012, at 14:15, Ruediger Pluem wrote:

 ap_strcmp_match seems to be a lot of overhead for just prefix matching a 
 string.
 How about
 
 strncmp(application/x-www-form-urlencoded, ct, 33)

Either way, shouldn't it be a case-insensitive match?

-- 
Nick Kew

RE: svn commit: r1372054 - in /httpd/httpd/trunk: CHANGES server/util.c

2012-08-13 Thread Plüm , Rüdiger , Vodafone Group


 -Original Message-
 From: Nick Kew [mailto:n...@webthing.com]
 Sent: Montag, 13. August 2012 13:11
 To: dev@httpd.apache.org
 Subject: Re: svn commit: r1372054 - in /httpd/httpd/trunk: CHANGES
 server/util.c
 
 
 On 12 Aug 2012, at 14:15, Ruediger Pluem wrote:
 
  ap_strcmp_match seems to be a lot of overhead for just prefix matching
 a string.
  How about
 
  strncmp(application/x-www-form-urlencoded, ct, 33)
 
 Either way, shouldn't it be a case-insensitive match?

Good point.

Regards

Rüdiger



core filters vs non-blocking socket (was Re: Fix for Windows bug#52476)

2012-08-13 Thread Joe Orton
On Fri, Aug 10, 2012 at 01:31:07PM -0400, Jeff Trawick wrote:
 We picked up that apr_socket_opt_set() from the async-dev branch with
 r327872, though the timeout calls in there were changed subsequently.
 I wonder if that call is stray and it doesn't get along with the
 timeout handling on Windows because of the SO_RCVTIMEO/SO_SENDTIMEO
 use on Windows, in lieu of non-blocking socket + poll like on Unix.
 
 At the time it was added, the new code was
 
 apr_socket_timeout_set(client_socket, 0)
 apr_socket_opt_set(client_socket, APR_SO_NONBLOCK, 1)
 
 (redundant unless there was some APR glitch at the time)

H, is this right?

For event, the listening socket, and hence the accepted socket, is 
always set to non-blocking in the MPM.

For non-event on Unix, the listening socket, and hence the accepted 
socket, is set to non-blocking IFF there are multiple listeners.

So that line is not redundant in the non-event, single listener 
configuration on Unix... no?

Regards, Joe


Re: Fix for Windows bug#52476

2012-08-13 Thread Apache Lounge
Also here it is running now without issues till now here with 
AcceptFilter-none+SSL


Steffen

-Original Message- 
From: Jeff Trawick

Sent: Friday, August 10, 2012 7:43 PM Newsgroups: gmane.comp.apache.devel
To: dev@httpd.apache.org
Subject: Re: Fix for Windows bug#52476

This patch is testing great so far with the AcceptFilter-none+SSL
scenario on Windows.

Index: server/core_filters.c
===
--- server/core_filters.c   (revision 1371377)
+++ server/core_filters.c   (working copy)
@@ -391,10 +391,6 @@
if (ctx == NULL) {
ctx = apr_pcalloc(c-pool, sizeof(*ctx));
net-out_ctx = (core_output_filter_ctx_t *)ctx;
-rv = apr_socket_opt_set(net-client_socket, APR_SO_NONBLOCK, 1);
-if (rv != APR_SUCCESS) {
-return rv;
-}
/*
 * Need to create tmp brigade with correct lifetime. Passing
 * NULL to apr_brigade_split_ex would result in a brigade

I'll run it through the test framework on Linux before committing.



Re: core filters vs non-blocking socket (was Re: Fix for Windows bug#52476)

2012-08-13 Thread Jeff Trawick
On Mon, Aug 13, 2012 at 8:32 AM, Joe Orton jor...@redhat.com wrote:
 On Fri, Aug 10, 2012 at 01:31:07PM -0400, Jeff Trawick wrote:
 We picked up that apr_socket_opt_set() from the async-dev branch with
 r327872, though the timeout calls in there were changed subsequently.
 I wonder if that call is stray and it doesn't get along with the
 timeout handling on Windows because of the SO_RCVTIMEO/SO_SENDTIMEO
 use on Windows, in lieu of non-blocking socket + poll like on Unix.

 At the time it was added, the new code was

 apr_socket_timeout_set(client_socket, 0)
 apr_socket_opt_set(client_socket, APR_SO_NONBLOCK, 1)

 (redundant unless there was some APR glitch at the time)

 H, is this right?

 For event, the listening socket, and hence the accepted socket, is
 always set to non-blocking in the MPM.

 For non-event on Unix, the listening socket, and hence the accepted
 socket, is set to non-blocking IFF there are multiple listeners.

But the underlying descriptor for the accepted gets set to
non-blocking soon after accept() (see below).


 So that line is not redundant in the non-event, single listener
 configuration on Unix... no?

 Regards, Joe

Background:  With the APR socket implementation on Unix, the
underlying socket descriptor is placed in non-blocking mode when the
application (httpd) sets a timeout = 0.

For a single listener worker configuration with mod_reqtimeout
disabled and the default Timeout of 60 seconds, here's what happens
w.r.t. blocking and timeouts on the connected sockets once
core_create_conn() is entered.  This is with the bug present (i.e.,
with the needless apr_socket_opt_set()).  At the time
core_create_conn() is entered, the apr_socket_t has timeout -1 and the
only option set is APR_TCP_NODELAY).

* core_pre_connection() sets timeout to 6000  (this makes the
underlying socket non-blocking on Unix)
* SSL needs to read but first calls flush and ap_core_output_filter()
makes the APR socket non-blocking (bug according to me ;) )
* also called from SSL flush, writev_nonblocking() sets timeout to 0
(that also ensures that the underlying socket is non-blocking)
* writev_nonblocking() restores the timeout (6000)
* also called from SSL flush, writev_nonblocking() sets timeout to 0
and then restores the timeout (6000)
* in some complicated flow from default_handler through SSL,
writev_nonblocking sets timeout to 0 then restores the timeout
* lingering_close() sets timeout to 200

So the underlying socket descriptor is still made non-blocking on Unix
even before the bug is encountered, as part of the timeout
implementation.  And that call to mark the socket non-blocking with
apr_socket_opt_set() is out of sync with the rest of the code that
sets timeout to 0 or 0 as necessary.  (And of course it is out of
sync in a way that exposes the difference on Windows.)

Does that explanation work for you?

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/


RE: core filters vs non-blocking socket (was Re: Fix for Windows bug#52476)

2012-08-13 Thread Plüm , Rüdiger , Vodafone Group


 -Original Message-
 From: Joe Orton [mailto:jor...@redhat.com]
 Sent: Montag, 13. August 2012 14:32
 To: dev@httpd.apache.org
 Subject: core filters vs non-blocking socket (was Re: Fix for Windows
 bug#52476)
 
 On Fri, Aug 10, 2012 at 01:31:07PM -0400, Jeff Trawick wrote:
  We picked up that apr_socket_opt_set() from the async-dev branch with
  r327872, though the timeout calls in there were changed subsequently.
  I wonder if that call is stray and it doesn't get along with the
  timeout handling on Windows because of the SO_RCVTIMEO/SO_SENDTIMEO
  use on Windows, in lieu of non-blocking socket + poll like on Unix.
 
  At the time it was added, the new code was
 
  apr_socket_timeout_set(client_socket, 0)
  apr_socket_opt_set(client_socket, APR_SO_NONBLOCK, 1)
 
  (redundant unless there was some APR glitch at the time)
 
 H, is this right?
 
 For event, the listening socket, and hence the accepted socket, is
 always set to non-blocking in the MPM.
 
 For non-event on Unix, the listening socket, and hence the accepted
 socket, is set to non-blocking IFF there are multiple listeners.
 
 So that line is not redundant in the non-event, single listener
 configuration on Unix... no?

Don't we switch to non-blocking in apr_socket_timeout_set if we set the timeout 
to 0?
And if we do a read with a timeout don't we do a poll with a timeout where it 
does not
matter whether the socket is blocking or non blocking?

Or did I get confused now completely?

Regards

Rüdiger



Re: core filters vs non-blocking socket (was Re: Fix for Windows bug#52476)

2012-08-13 Thread Jeff Trawick
On Mon, Aug 13, 2012 at 9:31 AM, Plüm, Rüdiger, Vodafone Group
ruediger.pl...@vodafone.com wrote:


 -Original Message-
 From: Joe Orton [mailto:jor...@redhat.com]
 Sent: Montag, 13. August 2012 14:32
 To: dev@httpd.apache.org
 Subject: core filters vs non-blocking socket (was Re: Fix for Windows
 bug#52476)

 On Fri, Aug 10, 2012 at 01:31:07PM -0400, Jeff Trawick wrote:
  We picked up that apr_socket_opt_set() from the async-dev branch with
  r327872, though the timeout calls in there were changed subsequently.
  I wonder if that call is stray and it doesn't get along with the
  timeout handling on Windows because of the SO_RCVTIMEO/SO_SENDTIMEO
  use on Windows, in lieu of non-blocking socket + poll like on Unix.
 
  At the time it was added, the new code was
 
  apr_socket_timeout_set(client_socket, 0)
  apr_socket_opt_set(client_socket, APR_SO_NONBLOCK, 1)
 
  (redundant unless there was some APR glitch at the time)

 H, is this right?

 For event, the listening socket, and hence the accepted socket, is
 always set to non-blocking in the MPM.

 For non-event on Unix, the listening socket, and hence the accepted
 socket, is set to non-blocking IFF there are multiple listeners.

 So that line is not redundant in the non-event, single listener
 configuration on Unix... no?

 Don't we switch to non-blocking in apr_socket_timeout_set if we set the 
 timeout to 0?

yes

apr_status_t apr_socket_timeout_set(apr_socket_t *sock, apr_interval_time_t t)
{
apr_status_t stat;

/* If our new timeout is non-negative and our old timeout was
 * negative, then we need to ensure that we are non-blocking.
 * Conversely, if our new timeout is negative and we had
 * non-negative timeout, we must make sure our socket is blocking.
 * We want to avoid calling fcntl more than necessary on the
 * socket.
 */
if (t = 0  sock-timeout  0) {
if (apr_is_option_set(sock, APR_SO_NONBLOCK) != 1) {
if ((stat = sononblock(sock-socketdes)) != APR_SUCCESS) {
return stat;
}
apr_set_option(sock, APR_SO_NONBLOCK, 1);
}
}
else if (t  0  sock-timeout = 0) {
if (apr_is_option_set(sock, APR_SO_NONBLOCK) != 0) {
if ((stat = soblock(sock-socketdes)) != APR_SUCCESS) {
return stat;
}
apr_set_option(sock, APR_SO_NONBLOCK, 0);
}
}
   ...

 And if we do a read with a timeout don't we do a poll with a timeout where it 
 does not
 matter whether the socket is blocking or non blocking?

from a high-level, yes; but there is some logic in APR to guess when
data is already available to read, and that requires that the socket
is non-blocking in case the guess is incorrect (that logic is
associated with the APR_INCOMPLETE_READ flag)


 Or did I get confused now completely?

no


 Regards

 Rüdiger




-- 
Born in Roswell... married an alien...
http://emptyhammock.com/


RE: core filters vs non-blocking socket (was Re: Fix for Windows bug#52476)

2012-08-13 Thread Plüm , Rüdiger , Vodafone Group


 -Original Message-
 From: Jeff Trawick [mailto:]
 Sent: Montag, 13. August 2012 15:35
 To: dev@httpd.apache.org
 Subject: Re: core filters vs non-blocking socket (was Re: Fix for
 Windows bug#52476)
 
 On Mon, Aug 13, 2012 at 9:31 AM, Plüm, Rüdiger, Vodafone Group
 ruediger.pl...@vodafone.com wrote:
 
  And if we do a read with a timeout don't we do a poll with a timeout
 where it does not
  matter whether the socket is blocking or non blocking?
 
 from a high-level, yes; but there is some logic in APR to guess when
 data is already available to read, and that requires that the socket
 is non-blocking in case the guess is incorrect (that logic is
 associated with the APR_INCOMPLETE_READ flag)

Thanks for the detailed clarification.

Regards

Rüdiger




Re: Fix for Windows bug#52476

2012-08-13 Thread Jeff Trawick
On Mon, Aug 13, 2012 at 8:58 AM, Apache Lounge i...@apachelounge.com wrote:
 Also here it is running now without issues till now here with
 AcceptFilter-none+SSL

awesome/thanks!


 Steffen

 -Original Message- From: Jeff Trawick
 Sent: Friday, August 10, 2012 7:43 PM Newsgroups: gmane.comp.apache.devel
 To: dev@httpd.apache.org
 Subject: Re: Fix for Windows bug#52476


 This patch is testing great so far with the AcceptFilter-none+SSL
 scenario on Windows.

 Index: server/core_filters.c
 ===
 --- server/core_filters.c   (revision 1371377)
 +++ server/core_filters.c   (working copy)
 @@ -391,10 +391,6 @@
 if (ctx == NULL) {
 ctx = apr_pcalloc(c-pool, sizeof(*ctx));
 net-out_ctx = (core_output_filter_ctx_t *)ctx;
 -rv = apr_socket_opt_set(net-client_socket, APR_SO_NONBLOCK, 1);
 -if (rv != APR_SUCCESS) {
 -return rv;
 -}
 /*
  * Need to create tmp brigade with correct lifetime. Passing
  * NULL to apr_brigade_split_ex would result in a brigade

 I'll run it through the test framework on Linux before committing.




-- 
Born in Roswell... married an alien...
http://emptyhammock.com/


Re: core filters vs non-blocking socket (was Re: Fix for Windows bug#52476)

2012-08-13 Thread Joe Orton
On Mon, Aug 13, 2012 at 09:27:08AM -0400, Jeff Trawick wrote:
 Does that explanation work for you?

Yes, perfectly, thanks for taking the time.  I stupidly forgot about the 
timeout calls... sorry!

Regards, Joe


How to align shm in an neat way?

2012-08-13 Thread Rainer Jung

Hi,

PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One 
of the three structs mapped into shm contains an apr_time_t member, 
which at least on Sparc is 8 Bytes, whereas for 32 bit builds long is 
only 4 Bytes.


Currently everything is aligned for 4 Bytes, so we get bus 
errors/crashes when trying to assign the apr_time_t to an address that 
is only divisible by 4 instead of 8.


I can easily reproduce the problem.

A possible solution is to pad the three structures SHMCBHeader, 
SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For 
Subcache and Index this is already true by coincidence, SHMCBHeader 
needs another 4 Bytes.


I wonder what the right solution is. In the patch

http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch

I hard coded the padding, but I don't really like it, because it breaks 
if members are added to the struct. I could add a sizeof() test during 
startup or probably even compilation to warn or err, if the padding is 
wrong.


I see several recipes for alignment using pragmas and attribute, but all 
of them are compiler specific.


One could also wrap the struct in a wrapped struct, so that one could 
use the sizeof() of the inner struct to determine the padding of the 
outer struct. That would make the code convoluted.


I checked other parts of the code, but couldn't find a simple solution. 
Any hints how to do this nicely?


Regards,

Rainer


Re: How to align shm in an neat way?

2012-08-13 Thread Jeff Trawick
On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung rainer.j...@kippdata.de wrote:
 Hi,

 PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
 the three structs mapped into shm contains an apr_time_t member, which at
 least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4 Bytes.

 Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
 when trying to assign the apr_time_t to an address that is only divisible by
 4 instead of 8.

 I can easily reproduce the problem.

 A possible solution is to pad the three structures SHMCBHeader,
 SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For Subcache
 and Index this is already true by coincidence, SHMCBHeader needs another 4
 Bytes.

 I wonder what the right solution is. In the patch

 http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch

 I hard coded the padding, but I don't really like it, because it breaks if
 members are added to the struct. I could add a sizeof() test during startup
 or probably even compilation to warn or err, if the padding is wrong.

 I see several recipes for alignment using pragmas and attribute, but all of
 them are compiler specific.

 One could also wrap the struct in a wrapped struct, so that one could use
 the sizeof() of the inner struct to determine the padding of the outer
 struct. That would make the code convoluted.

 I checked other parts of the code, but couldn't find a simple solution. Any
 hints how to do this nicely?

APR_ALIGN_DEFAULT?


 Regards,

 Rainer



-- 
Born in Roswell... married an alien...
http://emptyhammock.com/


Re: How to align shm in an neat way?

2012-08-13 Thread Rainer Jung

On 13.08.2012 18:32, Jeff Trawick wrote:

On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung rainer.j...@kippdata.de wrote:

Hi,

PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
the three structs mapped into shm contains an apr_time_t member, which at
least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4 Bytes.

Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
when trying to assign the apr_time_t to an address that is only divisible by
4 instead of 8.

I can easily reproduce the problem.

A possible solution is to pad the three structures SHMCBHeader,
SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For Subcache
and Index this is already true by coincidence, SHMCBHeader needs another 4
Bytes.

I wonder what the right solution is. In the patch

http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch

I hard coded the padding, but I don't really like it, because it breaks if
members are added to the struct. I could add a sizeof() test during startup
or probably even compilation to warn or err, if the padding is wrong.

I see several recipes for alignment using pragmas and attribute, but all of
them are compiler specific.

One could also wrap the struct in a wrapped struct, so that one could use
the sizeof() of the inner struct to determine the padding of the outer
struct. That would make the code convoluted.

I checked other parts of the code, but couldn't find a simple solution. Any
hints how to do this nicely?


APR_ALIGN_DEFAULT?


I think it doesn't solve this problem, does it? It only gives me a need 
way to round up sizes to multiples of 8 bytes.


Regards,

Rainer



Re: How to align shm in an neat way?

2012-08-13 Thread Jeff Trawick
On Mon, Aug 13, 2012 at 1:27 PM, Rainer Jung rainer.j...@kippdata.de wrote:
 On 13.08.2012 18:32, Jeff Trawick wrote:

 On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung rainer.j...@kippdata.de
 wrote:

 Hi,

 PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
 the three structs mapped into shm contains an apr_time_t member, which at
 least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4
 Bytes.

 Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
 when trying to assign the apr_time_t to an address that is only divisible
 by
 4 instead of 8.

 I can easily reproduce the problem.

 A possible solution is to pad the three structures SHMCBHeader,
 SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For
 Subcache
 and Index this is already true by coincidence, SHMCBHeader needs another
 4
 Bytes.

 I wonder what the right solution is. In the patch

 http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch

 I hard coded the padding, but I don't really like it, because it breaks
 if
 members are added to the struct. I could add a sizeof() test during
 startup
 or probably even compilation to warn or err, if the padding is wrong.

 I see several recipes for alignment using pragmas and attribute, but all
 of
 them are compiler specific.

 One could also wrap the struct in a wrapped struct, so that one could use
 the sizeof() of the inner struct to determine the padding of the outer
 struct. That would make the code convoluted.

 I checked other parts of the code, but couldn't find a simple solution.
 Any
 hints how to do this nicely?


 APR_ALIGN_DEFAULT?


 I think it doesn't solve this problem, does it? It only gives me a need way
 to round up sizes to multiples of 8 bytes.

It should be possible to use use APR_ALIGN_DEFAULT(sizeof(foo)) in
place of sizeof(foo), instead of adding explicit padding to the
structures. Any other pointer arithmetic which doesn't use sizeof(foo)
would also need to use the macro.



 Regards,

 Rainer




-- 
Born in Roswell... married an alien...
http://emptyhammock.com/


Re: How to align shm in an neat way?

2012-08-13 Thread Rainer Jung

On 13.08.2012 19:40, Jeff Trawick wrote:

On Mon, Aug 13, 2012 at 1:27 PM, Rainer Jung rainer.j...@kippdata.de wrote:

On 13.08.2012 18:32, Jeff Trawick wrote:


On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung rainer.j...@kippdata.de
wrote:


Hi,

PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
the three structs mapped into shm contains an apr_time_t member, which at
least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4
Bytes.

Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
when trying to assign the apr_time_t to an address that is only divisible
by
4 instead of 8.

I can easily reproduce the problem.

A possible solution is to pad the three structures SHMCBHeader,
SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For
Subcache
and Index this is already true by coincidence, SHMCBHeader needs another
4
Bytes.

I wonder what the right solution is. In the patch

http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch

I hard coded the padding, but I don't really like it, because it breaks
if
members are added to the struct. I could add a sizeof() test during
startup
or probably even compilation to warn or err, if the padding is wrong.

I see several recipes for alignment using pragmas and attribute, but all
of
them are compiler specific.

One could also wrap the struct in a wrapped struct, so that one could use
the sizeof() of the inner struct to determine the padding of the outer
struct. That would make the code convoluted.

I checked other parts of the code, but couldn't find a simple solution.
Any
hints how to do this nicely?



APR_ALIGN_DEFAULT?



I think it doesn't solve this problem, does it? It only gives me a need way
to round up sizes to multiples of 8 bytes.


It should be possible to use use APR_ALIGN_DEFAULT(sizeof(foo)) in
place of sizeof(foo), instead of adding explicit padding to the
structures. Any other pointer arithmetic which doesn't use sizeof(foo)
would also need to use the macro.


Understood. Unfortunately currently the code doesn't really care about 
alignment. So it just puts structs SHMCBHeader, SHMCBSubcache and an 
array of SHMCBIndex directly after each other in shm. No sizeof() involved.


So yes, option 3 would be to rewrite the code to calculate the 
gaps/padding between the structs using APR_ALIGN_DEFAULT() and adjust 
memory alignment in shm. I wanted to get around changing that part of 
the code ;)


Regards,

Rainer


Re: How to align shm in an neat way?

2012-08-13 Thread Rainer Jung

On 13.08.2012 21:02, Rainer Jung wrote:

On 13.08.2012 19:40, Jeff Trawick wrote:

On Mon, Aug 13, 2012 at 1:27 PM, Rainer Jung rainer.j...@kippdata.de
wrote:

On 13.08.2012 18:32, Jeff Trawick wrote:


On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung rainer.j...@kippdata.de
wrote:


Hi,

PR 53040 reveals, that mod_socache_shmcb has an alignment problem.
One of
the three structs mapped into shm contains an apr_time_t member,
which at
least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4
Bytes.

Currently everything is aligned for 4 Bytes, so we get bus
errors/crashes
when trying to assign the apr_time_t to an address that is only
divisible
by
4 instead of 8.

I can easily reproduce the problem.

A possible solution is to pad the three structures SHMCBHeader,
SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For
Subcache
and Index this is already true by coincidence, SHMCBHeader needs
another
4
Bytes.

I wonder what the right solution is. In the patch

http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch


I hard coded the padding, but I don't really like it, because it
breaks
if
members are added to the struct. I could add a sizeof() test during
startup
or probably even compilation to warn or err, if the padding is wrong.

I see several recipes for alignment using pragmas and attribute,
but all
of
them are compiler specific.

One could also wrap the struct in a wrapped struct, so that one
could use
the sizeof() of the inner struct to determine the padding of the outer
struct. That would make the code convoluted.

I checked other parts of the code, but couldn't find a simple
solution.
Any
hints how to do this nicely?



APR_ALIGN_DEFAULT?



I think it doesn't solve this problem, does it? It only gives me a
need way
to round up sizes to multiples of 8 bytes.


It should be possible to use use APR_ALIGN_DEFAULT(sizeof(foo)) in
place of sizeof(foo), instead of adding explicit padding to the
structures. Any other pointer arithmetic which doesn't use sizeof(foo)
would also need to use the macro.


Understood. Unfortunately currently the code doesn't really care about
alignment. So it just puts structs SHMCBHeader, SHMCBSubcache and an
array of SHMCBIndex directly after each other in shm. No sizeof() involved.

So yes, option 3 would be to rewrite the code to calculate the
gaps/padding between the structs using APR_ALIGN_DEFAULT() and adjust
memory alignment in shm. I wanted to get around changing that part of
the code ;)


I went the choose right alignment way now:

http://people.apache.org/~rjung/patches/mod_socache_shmcb-alignment.patch

It actually wasn't that complicated.

Regards,

Rainer