Bugs in zm_buffer.h and zm_buffer.cpp

[chrisj]

I am running a streaming (mjpg) remote camera and was periodically getting corrupted frames reported by zmc. I spent too much time tracking this down to what I think is a simple error which shows up in a few places, so I figure I should share a proposed fix. The error occurs when code in zm_buffer.h and zm_buffer.cpp make an invalid assumption that 'storage'=='head' and thus copy already-consumed data (from storage) instead of ready-to-consume data (from head).

In addition to that bug, the code in zm_buffer.h/cpp seems to consider the 'size' member to have two different meanings. Usually it is assumed to indicate how much live data is actually stored in the buffer, but at other times (such as allocation or after an Expand) it seems to try to convey some notion of how much space is available. This can cause some strange behavior. I think it is cleanest if 'size' indicates how much live data is available. The following patch fixes both the data corruption bug as well as keeping 'size' to mean the amount of data in the buffer, i.e. size == (tail - head).

Diffs from zm_buffer.h:

Code: Select all

*** /tmp/ZoneMinder-1.23.3/src/zm_buffer.h      Mon Feb 25 01:49:52 2008
--- ./zm_buffer.h       Tue May  6 23:55:44 2008
***************
*** 35,41 ****
        Buffer() : storage( 0 ), allocation( 0 ), size( 0 ), head( 0 ), tail( 0 )
        {
        }
!       Buffer( unsigned int p_size ) : allocation( p_size ), size( p_size )
        {
                head = storage = new unsigned char[allocation];
                tail = head;
--- 35,41 ----
        Buffer() : storage( 0 ), allocation( 0 ), size( 0 ), head( 0 ), tail( 0 )
        {
        }
!       Buffer( unsigned int p_size ) : allocation( p_size ), size( 0 )
        {
                head = storage = new unsigned char[allocation];
                tail = head;
***************
*** 49,55 ****
        Buffer( const Buffer &buffer ) : allocation( buffer.size ), size( buffer.size )
        {
                head = storage = new unsigned char[size];
!               memcpy( storage, buffer.storage, size );
                tail = head + size;
        }
        ~Buffer()
--- 49,55 ----
        Buffer( const Buffer &buffer ) : allocation( buffer.size ), size( buffer.size )
        {
                head = storage = new unsigned char[size];
!               memcpy( storage, buffer.head, size );
                tail = head + size;
        }
        ~Buffer()
***************
*** 78,84 ****
        unsigned int Assign( const unsigned char *p_storage, unsigned int p_size );
        unsigned int Assign( const Buffer &buffer )
        {
!               return( Assign( buffer.storage, buffer.size ) );
        }

        unsigned int Consume( unsigned int count )
--- 75,81 ----
        unsigned int Assign( const unsigned char *p_storage, unsigned int p_size );
        unsigned int Assign( const Buffer &buffer )
        {
!               return( Assign( buffer.head, buffer.size ) );
        }

        unsigned int Consume( unsigned int count )
***************
*** 122,127 ****
--- 119,125 ----
                Expand( p_size );
                memcpy( tail, p_storage, p_size );
                tail += p_size;
+               size += p_size;
                return( size );
        }
        unsigned int Append( const Buffer &buffer )

And diffs from zm_buffer.cpp (the actual problem that was biting me)

Code: Select all

***************
*** 53,65 ****
                unsigned char *new_storage = new unsigned char[allocation];
                if ( storage )
                {
!                       memcpy( new_storage, storage, size );
                        delete[] storage;
                }
                storage = new_storage;
                head = storage;
                tail = head + width;
        }
-       size += count;
        return( size );
  }
--- 57,68 ----
                unsigned char *new_storage = new unsigned char[allocation];
                if ( storage )
                {
!                       memcpy( new_storage, head, size );
                        delete[] storage;
                }
                storage = new_storage;
                head = storage;
                tail = head + width;
        }
        return( size );
  }

Note that I moved the update of 'size' out of Expand (which simply allocates more free space) and into Append (which actually writes new data into the buffer). This allows Expand to be used in other contexts without incorrectly updating the 'size' member.

Prior to this fix, I would get a 'Corrupt JPEG data' message every few minutes (depending on server load causing the buffer to start filling up), causing zmc to exit/restart. I'm not sure I've got all of the buffering corner cases fixed, but it's been running 10x longer than before without any errors so far, so I think it's an improvement.

[mor]

Ah-hah! Thanks for this.

I'm ashamed to say I hacked around this in zmc by changing it simply to not exit if it got a correct image. I thought it was my cameras producing that junk

I'll put this patch into my source tree.

Ps: You may want to consider using 'diff -u' to generate patches. It makes them far easier to read.

Edit: this has a new bug in it somewhere. It causes images to be inserted out of sequence somehow. I'll try and track this down later.

[chrisj]

Ah-hah! Thanks for this.
Ps: You may want to consider using 'diff -u' to generate patches. It makes them far easier to read.

Yeah, good idea. It was actually a bit of a pain to generate the diffs w.r.t. the original code since I had hacked on it so much to debug it. When I get a chance I will re-do the fix on a clean code base and post an update.

Edit: this has a new bug in it somewhere. It causes images to be inserted out of sequence somehow. I'll try and track this down later.

That's odd. I've been running with this for over a week now and I haven't seen that behavior at all. I have seen the occasional 'Corrupt JPEG' since I made the fix, but it's down to maybe once per day on my main server (vs. once every few minutes). My other box is a poor overloaded Pentium M and it shows up a bit more frequently there (once every few hours), so it seems like it might still be a buffering problem somewhere that is exacerbated by higher loads.