corrupt blocks overwrite previously verified OK blocks

[cfpp2p]

I can verify that there is indeed a bug in webseed.c where corrupt blocks overwrite previously deemed verified OK block of a piece. I have test torrents that will consistently reproduce verified corrupt blocks after 100% completion, and then a state of less than 100%. The fix is having tr_torrentPieceIsComplete() test before the two instances of tr_cacheWriteBlock() in webseed.c

This fault was introduced with r12539. See #4649 for related information.

something like this will do it

static void
write_block_func (void * vdata)
{
  struct write_block_data * data = vdata;
  struct tr_webseed * w = data->webseed;
  struct evbuffer * buf = data->content;
  struct tr_torrent * tor;

  tor = tr_torrentFindFromId (data->session, data->torrent_id);
  if (tor != NULL)
    {
      const uint32_t block_size = tor->blockSize;
      uint32_t len = evbuffer_get_length (buf);
      const uint32_t offset_end = data->block_offset + len;
      tr_cache * cache = data->session->cache;
      const tr_piece_index_t piece = data->piece_index;
     
      if (!tr_torrentPieceIsComplete (tor, piece))
   {
     while (len > 0)
       {
         const uint32_t bytes_this_pass = MIN (len, block_size);
         tr_cacheWriteBlock (cache, tor, piece, offset_end - len, bytes_this_pass, buf);
         len -= bytes_this_pass;
       }
      
     tr_bitfieldAdd (&w->blame, piece);
     fire_client_got_blocks (tor, w, data->block_index, data->count);
   }
    }

  evbuffer_free (buf);
  tr_free (data);
}

static void
web_response_func (tr_session    * session,
         char       * task_addr,
         bool         aborted,
                   bool            did_connect UNUSED,
                   bool            did_timeout,
                   long            response_code,
                   const void    * response UNUSED,
                   size_t          response_byte_count UNUSED,
                   void          * vtask)
{
...

           else
            {
              if (buf_len && !tr_torrentPieceIsComplete (tor, t->piece_index))
                {
                  /* on_content_changed () will not write a block if it is smaller than
                     the torrent's block size, i.e. the torrent's very last block */
                  tr_cacheWriteBlock (session->cache, tor,
                                      t->piece_index, t->piece_offset + bytes_done,
                                      buf_len, t->content);
        tr_bitfieldAdd (&w->blame, t->piece_index);
                  fire_client_got_blocks (tor, t->webseed,
                                          t->block + t->blocks_done, 1);
                }

[x190]

One should apologize for cf's lack of attention to proper formatting. Also, the tr_bitfieldAdd() lines in each code block and the "char * task_addr, bool aborted," lines in the second block are from my upcoming webseed patch, which also means "bool did_timeout" should be "bool did_timeout UNUSED" for this ticket.

See #5923 for a patch for this and many more webseed issues.

[cfpp2p]

x190's thorough testing found the bug, and regardless the bug exists in current trunk and prior versions back to at least r12539. The original fix that I came up with, in its simplest form, fixes trunk as well as x190's 'upcoming webseed patch'. Click the Unified tab below to see in well formatted view.

J:/T~~/5912/webseed.c

@@ -162 +162 @@
       tr_cache * cache = data->session->cache;
       const tr_piece_index_t piece = data->piece_index;
 
-      while (len > 0)
+      if (!tr_torrentPieceIsComplete (tor, piece))
         {
-          const uint32_t bytes_this_pass = MIN (len, block_size);
-          tr_cacheWriteBlock (cache, tor, piece, offset_end - len, bytes_this_pass, buf);
-          len -= bytes_this_pass;
+          while (len > 0)
+            {
+              const uint32_t bytes_this_pass = MIN (len, block_size);
+              tr_cacheWriteBlock (cache, tor, piece, offset_end - len, bytes_this_pass, buf);
+              len -= bytes_this_pass;
+            }
+          fire_client_got_blocks (tor, w, data->block_index, data->count);
         }
-
-      fire_client_got_blocks (tor, w, data->block_index, data->count);
     }
 
   evbuffer_free (buf);
@@ -422 +424 @@
             }
             else
             {
-              if (buf_len)
+              if (buf_len && !tr_torrentPieceIsComplete (tor, t->piece_index))
                 {
                   /* on_content_changed () will not write a block if it is smaller than
                      the torrent's block size, i.e. the torrent's very last block */ 

The bug can easily be reproduced and likely produced some of the complaints dictated in #4649

[rsully]

Does this only affect users using web seeds, what is required to reproduce it?

[cfpp2p]

Replying to rsully:

Does this only affect users using web seeds, what is required to reproduce it?

Yes, web seeds.

The requirements to reproduce are at least one webseed that sends corrupt block(s), and at least one other peer (webseed or regular peer) that sends correct data for the same block.

Then a peer with correct block sends the block and transmission accepts and verifies that block as good. But then the webseed thread comes around and the same block is downloaded from the webseed that sends a bad block which is then written even though the piece was previously deemed complete.

It may not happen every time, but if the required elements of the situation are there it will happen frequently and easily enough for proof.

I did quite a lot of testing and research with this issue and other webseed issues. It's been a few weeks now so I can't really remember any better than that right now.

[x190]

Patch: https://trac.transmissionbt.com/ticket/5923#comment:9

[mike.dld]

Committed as r14550, thanks.