Bug #45093 Wrong logic in backup stream library in bstream_next_chunk() function
Submitted: 26 May 2009 9:02 Modified: 9 Jul 2009 16:39
Reporter: Rafal Somla Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: Backup Severity:S3 (Non-critical)
Version:6.0-backup OS:Any
Assigned to: Rafal Somla CPU Architecture:Any
Tags: teamtree

[26 May 2009 9:02] Rafal Somla
Description:
Function bstream_next_chunk() works incorrectly if the current chunk spans more than one fragment.

Function bstream_next_chunk() in stream_v1_transport.c should move backup stream position to the beginning of the next chunk. The logic is complex, performing various actions depending on the internal state of the stream. However, the final result is that stream is moved to the begining of next fragment in the stream (the call to load_next_fragment()). This is not correct if the current fragment is not the last fragment of the current chunk. In that case, more than one fragment must be skipped but there is no logic to do this.

How to repeat:
Code inspection. Hope to provide test code soon.
[26 May 2009 10:52] Rafal Somla
Test program showing the problem.

Attachment: bug45093.cc (application/octet-stream, text), 5.99 KiB.

[26 May 2009 10:52] Rafal Somla
Makefile for the test program.

Attachment: bug45093.make (application/octet-stream, text), 170 bytes.

[26 May 2009 10:55] Rafal Somla
To see the problem, copy attached files bug45093.{cc,make} to sql/backup dir and run "make -f bug45093.make". This should build bug45093 test program.

This program writes 2 chunks of data filled with 0x5C byte, but with first byte in each chunk set to 0xC5. Then it reads part of first chunk, skips to next chunk and reads first few bytes from it. If the two first bytes are not equal to 0xC5, 0x5C then something is wrong.
[16 Jun 2009 13:19] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/76390

2824 Rafal Somla	2009-06-16
      Bug #45093 - Wrong logic in backup stream library in bstream_next_chunk()...
      
      Temporary commit - please ignore.
[17 Jun 2009 5:12] Rafal Somla
Setting to "in progress" - a final patch still needs to be prepared.
[19 Jun 2009 13:33] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/76710

2827 Rafal Somla	2009-06-19
      Bug #45093 - Wrong logic in backup stream library in bstream_next_chunk()...
      
      The problem in bstream_next_chunk() was that the logic did not take into
      account the fact that there can be more than one fragment of a chunk left
      in the stream - only the current fragment was skipped.
      
      This patch adds a loop which skips all remaining fragments of the current
      chunk in order to move to the next one. It also fixes few other small 
      problems detected when I worked on this issue.
     @ sql/backup/stream_v1.c
        Small fixes:
         - in bstream_wr_db_catalogue() function, db_iterator is created and thus 
           db_iterator_free() function should be called to free it.
         - no need to skip tables in the loop iterating over per-table items: 
           prbably a copy-paste error.
     @ sql/backup/stream_v1_transport.c
        The main fix is in bstream_next_chunk() function where a loop is added. This 
        loop will skip all remaining fragments of the current chunk, so that stream
        is positioned at the beginning of next chunk.
        
        Apart from that:
        - Fix definition of FR_LEN_MASK to specify bits explicitly. Otherwise the
        bit pattern could vary depending on the platform.
        - Fix logic detecting end-of-chunk in load_next_fragment() function.
        - Make check_end() function correctly set stream state when end-of-chunk
          is detected.
[24 Jun 2009 19:30] Chuck Bell
Approved pending minor changes.
[25 Jun 2009 12:32] Jørgen Løland
Good to push
[26 Jun 2009 6:29] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/77269

2833 Rafal Somla	2009-06-26
      Bug #45093 - Wrong logic in backup stream library in bstream_next_chunk()...
      
      The problem in bstream_next_chunk() was that the logic did not take into
      account the fact that there can be more than one fragment of a chunk left
      in the stream - only the current fragment was skipped.
      
      This patch adds a loop which skips all remaining fragments of the current
      chunk in order to move to the next one. It also fixes few other small 
      problems detected when I worked on this issue.
     @ sql/backup/stream_v1.c
        Small fixes:
         - in bstream_wr_db_catalogue() function, db_iterator is created and thus 
           db_iterator_free() function should be called to free it.
         - no need to skip tables in the loop iterating over per-table items: 
           prbably a copy-paste error.
     @ sql/backup/stream_v1_transport.c
        The main fix is in bstream_next_chunk() function where a loop is added. This 
        loop will skip all remaining fragments of the current chunk, so that stream
        is positioned at the beginning of next chunk.
        
        Apart from that:
        - Fix definition of FR_LEN_MASK to specify bits explicitly. Otherwise the
        bit pattern could vary depending on the platform.
        - Fix logic detecting end-of-chunk in load_next_fragment() function.
        - Make check_end() function correctly set stream state when end-of-chunk
          is detected.
[26 Jun 2009 6:30] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/77270

2833 Rafal Somla	2009-06-26
      Bug #45093 - Wrong logic in backup stream library in bstream_next_chunk()...
      
      The problem in bstream_next_chunk() was that the logic did not take into
      account the fact that there can be more than one fragment of a chunk left
      in the stream - only the current fragment was skipped.
      
      This patch adds a loop which skips all remaining fragments of the current
      chunk in order to move to the next one. It also fixes few other small 
      problems detected when I worked on this issue.
     @ sql/backup/stream_v1.c
        Small fixes:
         - in bstream_wr_db_catalogue() function, db_iterator is created and thus 
           db_iterator_free() function should be called to free it.
         - no need to skip tables in the loop iterating over per-table items: 
           probably a copy-paste error.
     @ sql/backup/stream_v1_transport.c
        The main fix is in bstream_next_chunk() function where a loop is added. This 
        loop will skip all remaining fragments of the current chunk, so that stream
        is positioned at the beginning of next chunk.
        
        Apart from that:
        - Fix definition of FR_LEN_MASK to specify bits explicitly. Otherwise the
        bit pattern could vary depending on the platform.
        - Fix logic detecting end-of-chunk in load_next_fragment() function.
        - Make check_end() function correctly set stream state when end-of-chunk
          is detected.
[26 Jun 2009 7:15] Rafal Somla
Pushed to backup team tree.
revision-id:rafal.somla@sun.com-20090626062950-uux07ubdv59oep93
[7 Jul 2009 14:09] Bugs System
Pushed into 5.4.4-alpha (revid:alik@sun.com-20090707140519-svplog8kcfejbzbe) (version source revid:rafal.somla@sun.com-20090626062950-uux07ubdv59oep93) (merge vers: 5.4.4-alpha) (pib:11)
[9 Jul 2009 7:36] Bugs System
Pushed into 5.4.4-alpha (revid:alik@sun.com-20090707140519-svplog8kcfejbzbe) (version source revid:rafal.somla@sun.com-20090626062950-uux07ubdv59oep93) (merge vers: 5.4.4-alpha) (pib:11)
[9 Jul 2009 16:39] Paul DuBois
Not in any released version. No changelog entry needed.