Re: Posix sems still not recommended?

[venkatnv]

Well ... i think we found the root cause, in one of the libraries being used,
the mutex was not being initialized. Thanks!


venkatnv wrote:
> 
> We are observing issues with pthread Mutexes on Apache22/Solaris10. Not
> sure if this is relevant to this thread, but would appreciate any  inputs.
> 
> - We are running Apache22 in Worker mode. Apache22 is compiled with gcc346
> on Solaris10
> - We are having a custom module (DSO) loaded with Apache.
> 
> On stress test, we see that a mutex is not working as intended.
> (pthread_mutex_lock)
> To be precise, we are seeing core dumps and further investigation revealed
> that there are two threads that have acquired a lock using
> pthread_mutex_lock, a the same time. 
> 
> Please note that we do not see this behavior on Apache2. This occurs only
> with Apache22. Has anyone come across a similar situation. Any help in
> narrowing down the cause would be greatly appreciated!
> 
> Regards,
> Venkat.
> 
> 
> Rainer Jung-3 wrote:
>> 
>> On 30.03.2009 20:58, Jeff Trawick wrote:
>>> On Mon, Mar 30, 2009 at 2:33 PM, Jeff Trawick >> > wrote:
>>>
>>>
>>>
>>> On Mon, Mar 30, 2009 at 2:07 PM, Jim Jagielski >> > wrote:
>>>
>>> Anyone know if:
>>>
>>> # POSIX semaphores and cross-process pthread mutexes are not # used
>>> by default since they have less desirable behaviour when # e.g. a
>>> process holding the mutex segfaults.
>>>
>>> is still applicable, at least for posix sems?
>>>
>>>
>>> AFAIK, the Solaris-specific recovery logic for cross-process pthread
>>> mutexes has been working reliably for a long time, but with the
>>> current wind direction APR is choosing fcntl(), which has sysdef
>>> implementations on that
>>>
>>>
>>> ugh; "sysdef implications"
>> 
>> and quite often shows EDEADLOCK, even when you can prove there can't be
>> one. Especially when starting to use more than one lock of that type
>> (e.g. when SSL comes into the game).
>> 
>>> platform.
>>>
>>> no clues here about the POSIX semaphores
>> 
>> I would be much interested in an answer as well. Because of the
>> EDEADLOCK problems I did suggest using the pthread based mutex on
>> Solaris for a while to people and got no problem reports. But what
>> experience do others have?
>> 
>> In a related thread on the Tomcat users list about mod_jk I wrote in
>> February:
>> 
>>I now did some searching and it turns out that the implementation of
>>pthread mutexes for Solaris 10 has very recently changed quite a bit.
>>So all speculations about improved pthread mutex behaviour
>>(especially for "robust" mutexes) in the last years might have become
>>obsolete.
>> 
>>The new implementation is contained in Solaris kernel patch 137137-09
>>and most likely also in Solaris 10 Update 6 (10/08). I didn't check,
>>whether that update simply contains the kernel patch or the fix is
>>included independently.
>> 
>>Some detail is logged in Sunsolve under the bug IDs
>> 
>>6296770 2160259 6664275 6697344 6729759 6564706
>> 
>> Regards,
>> 
>> Rainer
>> 
>> 
> 
> 

-- 
View this message in context: 
http://www.nabble.com/Posix-sems-still-not-recommended--tp22789262p23222108.html
Sent from the APR Dev (Apache Portable Runtime) mailing list archive at 
Nabble.com.

Re: Posix sems still not recommended?

[Jeff Trawick]

On Fri, Apr 24, 2009 at 2:10 PM, venkatnv  wrote:

>
> Well ... i think we found the root cause, in one of the libraries being
> used,
> the mutex was not being initialized. Thanks!
>
>
> venkatnv wrote:
> >
> > We are observing issues with pthread Mutexes on Apache22/Solaris10. Not
> > sure if this is relevant to this thread, but would appreciate any
>  inputs.
>

Thanks for following up on that ;)  It did seem to be a different issue
altogether.

Good luck!

Buffer overflow in apr_brigade_vprintf() ?

[C. Michael Pilato]

[Please Cc: me in responses -- I think I still have APR commit privs, but
 I'm not active here and not subscribed to the mailing lists.]

In the past couple of weeks, I've seen two different reports of what appears
to be corruption in the stream of data transmitted by Subversion's
mod_dav_svn through Apache and back to the Subversion client.  What is seen
client-side is an opening XML tag, a truncated bit of CDATA "inside" the
tag, and then a missing XML closing tag.  The problem seems to occur with
magically sized chunks of data, so it can be hard to reproduce[1].

Here are the relevant pieces of the call stack:

mod_dav_svn/reports/replay.c's change_file_or_dir_prop() function contains
the following (which is base64-encoding Subversion file and directory
properties, and tossing them into an XML REPORT request response):

   const svn_string_t *enc_value = svn_base64_encode_string2(value, TRUE,
 pool);
   SVN_ERR(dav_svn__send_xml
(eb->bb, eb->output,
 "%s" DEBUG_CR,
 file_or_dir, qname, enc_value->data, file_or_dir));

dav_svn__send_xml() is a wrapper around apr_brigade_vprintf().

As you know, apr_brigade_vprintf() (in buckets/apr_brigade.c) looks like so:

APU_DECLARE(apr_status_t) apr_brigade_vprintf(apr_bucket_brigade *b,
  apr_brigade_flush flush,
  void *ctx,
  const char *fmt, va_list va)
{
/* the cast, in order of appearance */
struct brigade_vprintf_data_t vd;
char buf[APR_BUCKET_BUFF_SIZE];
int written;

vd.vbuff.curpos = buf;
vd.vbuff.endpos = buf + APR_BUCKET_BUFF_SIZE;
vd.b = b;
vd.flusher = &flush;
vd.ctx = ctx;
vd.cbuff = buf;

written = apr_vformatter(brigade_flush, &vd.vbuff, fmt, va);

if (written == -1) {
  return -1;
}

/* tack on null terminator to remaining string */
*(vd.vbuff.curpos) = '\0';

/* write out what remains in the buffer */
return apr_brigade_write(b, flush, ctx, buf, vd.vbuff.curpos - buf);
}

The function apr_vformatter() uses the buffer "buf" to format the string.
This function in turn uses the macro INS_CHAR to add characters to the
buffer.  INS_CHAR is defined like this:

#define INS_CHAR(c, sp, bep, cc)\
{   \
if (sp) {   \
if (sp >= bep) {\
vbuff->curpos = sp; \
if (flush_func(vbuff))  \
return -1;  \
sp = vbuff->curpos; \
bep = vbuff->endpos;\
}   \
*sp++ = (c);\
}   \
cc++;   \
}

So, when the macro is executed to add a new character to the buffer and the
buffer is full, the flush function is called to make room for the new
character, and then the character is added.  Of course, if the buffer has
room for exactly one more character, it is not flushed, the character is
added, and the current position of the buffer is at its end (which is
actually one byte beyond the space allocated for the buffer).

After the call to apr_vformatter(), there will be stuff in the buffer.  In
the special case above, the buffer may be perfectly full (perhaps after
having been flushed one or more times, but still full now).  Then, without
checking for that condition, this line is executed:

/* tack on null terminator to remaining string */
*(vd.vbuff.curpos) = '\0';

Uh-oh.  Buffer overflow!

Our CollabNet engineer is proposing a simple fix:  defining 'buf' inside
apr_brigade_vprintf() like so:

char buf[APR_BUCKET_BUFF_SIZE + 1]

(Note the "+ 1" to make room for that pesky NULL byte.)

But I'm wondering if an equally correct fix would be to simply not tack the
'\0' onto the buffer at all.  Doesn't apr_brigade_write() accept both the
buffer and the number of bytes to write?  Does it really need a
null-terminated string, especially considering that its input could be
arbitrary binary data?  Other calls to it pass things like "str" and
"strlen(str)", which would ignore the NULL byte in "str".

[1]
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&viewType=browseAll&dsMessageId=1745697

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand



signature.asc
Description: OpenPGP digital signature

Re: Buffer overflow in apr_brigade_vprintf() ?

[Ruediger Pluem]

On 04/24/2009 10:10 PM, C. Michael Pilato wrote:
> [Please Cc: me in responses -- I think I still have APR commit privs, but
>  I'm not active here and not subscribed to the mailing lists.]
> 
> In the past couple of weeks, I've seen two different reports of what appears
> to be corruption in the stream of data transmitted by Subversion's
> mod_dav_svn through Apache and back to the Subversion client.  What is seen
> client-side is an opening XML tag, a truncated bit of CDATA "inside" the
> tag, and then a missing XML closing tag.  The problem seems to occur with
> magically sized chunks of data, so it can be hard to reproduce[1].
> 
> Here are the relevant pieces of the call stack:
> 
> mod_dav_svn/reports/replay.c's change_file_or_dir_prop() function contains
> the following (which is base64-encoding Subversion file and directory
> properties, and tossing them into an XML REPORT request response):
> 
>const svn_string_t *enc_value = svn_base64_encode_string2(value, TRUE,
>  pool);
>SVN_ERR(dav_svn__send_xml
> (eb->bb, eb->output,
>  "%s" DEBUG_CR,
>  file_or_dir, qname, enc_value->data, file_or_dir));
> 
> dav_svn__send_xml() is a wrapper around apr_brigade_vprintf().
> 
> As you know, apr_brigade_vprintf() (in buckets/apr_brigade.c) looks like so:
> 
> APU_DECLARE(apr_status_t) apr_brigade_vprintf(apr_bucket_brigade *b,
>   apr_brigade_flush flush,
>   void *ctx,
>   const char *fmt, va_list va)
> {
> /* the cast, in order of appearance */
> struct brigade_vprintf_data_t vd;
> char buf[APR_BUCKET_BUFF_SIZE];
> int written;
> 
> vd.vbuff.curpos = buf;
> vd.vbuff.endpos = buf + APR_BUCKET_BUFF_SIZE;
> vd.b = b;
> vd.flusher = &flush;
> vd.ctx = ctx;
> vd.cbuff = buf;
> 
> written = apr_vformatter(brigade_flush, &vd.vbuff, fmt, va);
> 
> if (written == -1) {
>   return -1;
> }
> 
> /* tack on null terminator to remaining string */
> *(vd.vbuff.curpos) = '\0';
> 
> /* write out what remains in the buffer */
> return apr_brigade_write(b, flush, ctx, buf, vd.vbuff.curpos - buf);
> }
> 
> The function apr_vformatter() uses the buffer "buf" to format the string.
> This function in turn uses the macro INS_CHAR to add characters to the
> buffer.  INS_CHAR is defined like this:
> 
> #define INS_CHAR(c, sp, bep, cc)\
> {   \
> if (sp) {   \
> if (sp >= bep) {\
> vbuff->curpos = sp; \
> if (flush_func(vbuff))  \
> return -1;  \
> sp = vbuff->curpos; \
> bep = vbuff->endpos;\
> }   \
> *sp++ = (c);\
> }   \
> cc++;   \
> }
> 
> So, when the macro is executed to add a new character to the buffer and the
> buffer is full, the flush function is called to make room for the new
> character, and then the character is added.  Of course, if the buffer has
> room for exactly one more character, it is not flushed, the character is
> added, and the current position of the buffer is at its end (which is
> actually one byte beyond the space allocated for the buffer).
> 
> After the call to apr_vformatter(), there will be stuff in the buffer.  In
> the special case above, the buffer may be perfectly full (perhaps after
> having been flushed one or more times, but still full now).  Then, without
> checking for that condition, this line is executed:
> 
> /* tack on null terminator to remaining string */
> *(vd.vbuff.curpos) = '\0';
> 
> Uh-oh.  Buffer overflow!
> 
> Our CollabNet engineer is proposing a simple fix:  defining 'buf' inside
> apr_brigade_vprintf() like so:
> 
> char buf[APR_BUCKET_BUFF_SIZE + 1]
> 
> (Note the "+ 1" to make room for that pesky NULL byte.)
> 
> But I'm wondering if an equally correct fix would be to simply not tack the
> '\0' onto the buffer at all.  Doesn't apr_brigade_write() accept both the
> buffer and the number of bytes to write?  Does it really need a
> null-terminated string, especially considering that its input could be
> arbitrary binary data?  Other calls to it pass things like "str" and
> "strlen(str)", which would ignore the NULL byte in "str".

Thanks for the detailed analysis. IMHO the best fix is to simply remove
 *(vd.vbuff.curpos) = '\0';
apr_brigade_write does not expect a 'string' that means a sequence
of bytes with '\0' marking its end. It expects a buffer of a given length.
The way it i

Re: Buffer overflow in apr_brigade_vprintf() ?

[Jeff Trawick]

On Fri, Apr 24, 2009 at 4:10 PM, C. Michael Pilato wrote:

> [Please Cc: me in responses -- I think I still have APR commit privs, but
>  I'm not active here and not subscribed to the mailing lists.]
>
> In the past couple of weeks, I've seen two different reports of what
> appears
> to be corruption in the stream of data transmitted by Subversion's
> mod_dav_svn through Apache and back to the Subversion client.  What is seen
> client-side is an opening XML tag, a truncated bit of CDATA "inside" the
> tag, and then a missing XML closing tag.  The problem seems to occur with
> magically sized chunks of data, so it can be hard to reproduce[1].
>
> Here are the relevant pieces of the call stack:
>
> mod_dav_svn/reports/replay.c's change_file_or_dir_prop() function contains
> the following (which is base64-encoding Subversion file and directory
> properties, and tossing them into an XML REPORT request response):
>
>   const svn_string_t *enc_value = svn_base64_encode_string2(value, TRUE,
> pool);
>   SVN_ERR(dav_svn__send_xml
>(eb->bb, eb->output,
> "%s" DEBUG_CR,
> file_or_dir, qname, enc_value->data, file_or_dir));
>
> dav_svn__send_xml() is a wrapper around apr_brigade_vprintf().
>
> As you know, apr_brigade_vprintf() (in buckets/apr_brigade.c) looks like
> so:
>
> APU_DECLARE(apr_status_t) apr_brigade_vprintf(apr_bucket_brigade *b,
>  apr_brigade_flush flush,
>  void *ctx,
>  const char *fmt, va_list va)
> {
>/* the cast, in order of appearance */
>struct brigade_vprintf_data_t vd;
>char buf[APR_BUCKET_BUFF_SIZE];
>int written;
>
>vd.vbuff.curpos = buf;
>vd.vbuff.endpos = buf + APR_BUCKET_BUFF_SIZE;
>vd.b = b;
>vd.flusher = &flush;
>vd.ctx = ctx;
>vd.cbuff = buf;
>
>written = apr_vformatter(brigade_flush, &vd.vbuff, fmt, va);
>
>if (written == -1) {
>  return -1;
>}
>
>/* tack on null terminator to remaining string */
>*(vd.vbuff.curpos) = '\0';
>
>/* write out what remains in the buffer */
>return apr_brigade_write(b, flush, ctx, buf, vd.vbuff.curpos - buf);
> }
>
> The function apr_vformatter() uses the buffer "buf" to format the string.
> This function in turn uses the macro INS_CHAR to add characters to the
> buffer.  INS_CHAR is defined like this:
>
> #define INS_CHAR(c, sp, bep, cc)\
> {   \
>if (sp) {   \
>if (sp >= bep) {\
>vbuff->curpos = sp; \
>if (flush_func(vbuff))  \
>return -1;  \
>sp = vbuff->curpos; \
>bep = vbuff->endpos;\
>}   \
>*sp++ = (c);\
>}   \
>cc++;   \
> }
>
> So, when the macro is executed to add a new character to the buffer and the
> buffer is full, the flush function is called to make room for the new
> character, and then the character is added.  Of course, if the buffer has
> room for exactly one more character, it is not flushed, the character is
> added, and the current position of the buffer is at its end (which is
> actually one byte beyond the space allocated for the buffer).
>
> After the call to apr_vformatter(), there will be stuff in the buffer.  In
> the special case above, the buffer may be perfectly full (perhaps after
> having been flushed one or more times, but still full now).  Then, without
> checking for that condition, this line is executed:
>
>/* tack on null terminator to remaining string */
>*(vd.vbuff.curpos) = '\0';
>
> Uh-oh.  Buffer overflow!
>
> Our CollabNet engineer is proposing a simple fix:  defining 'buf' inside
> apr_brigade_vprintf() like so:
>
>char buf[APR_BUCKET_BUFF_SIZE + 1]
>
> (Note the "+ 1" to make room for that pesky NULL byte.)
>
> But I'm wondering if an equally correct fix would be to simply not tack the
> '\0' onto the buffer at all.  Doesn't apr_brigade_write() accept both the
> buffer and the number of bytes to write?  Does it really need a
> null-terminated string, especially considering that its input could be
> arbitrary binary data?  Other calls to it pass things like "str" and
> "strlen(str)", which would ignore the NULL byte in "str".


I agree; it won't have the terminating '\0' once it is written to the
brigade, and apr_brigade_write() doesn't need it.

Re: Buffer overflow in apr_brigade_vprintf() ?

[Ruediger Pluem]

On 04/24/2009 10:10 PM, C. Michael Pilato wrote:

> 
> /* tack on null terminator to remaining string */
> *(vd.vbuff.curpos) = '\0';
> 
> Uh-oh.  Buffer overflow!
> 
> Our CollabNet engineer is proposing a simple fix:  defining 'buf' inside
> apr_brigade_vprintf() like so:
> 
> char buf[APR_BUCKET_BUFF_SIZE + 1]
> 
> (Note the "+ 1" to make room for that pesky NULL byte.)
> 
> But I'm wondering if an equally correct fix would be to simply not tack the
> '\0' onto the buffer at all.  Doesn't apr_brigade_write() accept both the
> buffer and the number of bytes to write?  Does it really need a
> null-terminated string, especially considering that its input could be
> arbitrary binary data?  Other calls to it pass things like "str" and
> "strlen(str)", which would ignore the NULL byte in "str".
> 
> [1]
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&viewType=browseAll&dsMessageId=1745697
> 

Fixed in r768417 (http://svn.apache.org/viewvc?view=rev&revision=768417).

Regards

Rüdiger

Re: Buffer overflow in apr_brigade_vprintf() ?

[C. Michael Pilato]

C. Michael Pilato wrote:
> [Please Cc: me in responses -- I think I still have APR commit privs, but
>  I'm not active here and not subscribed to the mailing lists.]
> 
> In the past couple of weeks, I've seen two different reports of what appears
> to be corruption in the stream of data transmitted by Subversion's
> mod_dav_svn through Apache and back to the Subversion client.  What is seen
> client-side is an opening XML tag, a truncated bit of CDATA "inside" the
> tag, and then a missing XML closing tag.  The problem seems to occur with
> magically sized chunks of data, so it can be hard to reproduce[1].

[...]

Just to bring this to closure, the bug was fixed by committing the removal
of the code that tacks the NULL byte onto a possibly-already-full buffer:

   http://svn.apache.org/viewvc?view=rev&revision=768417

(Thanks, Ruediger and Jeff!)

-- 
C. Michael Pilato 
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand



signature.asc
Description: OpenPGP digital signature