Opened 8 years ago

Closed 8 years ago

#5734 closed Bug (duplicate)

transmission accepts invalid piece sized torrents when piece size less than MAX_BLOCK_SIZE

Reported by: cfpp2p Owned by:
Priority: Normal Milestone: None Set
Component: Transmission Version: 2.84
Severity: Normal Keywords:
Cc:

Description

Transmission accepts for download torrents with a NON power of two piece size when the piece size is less than MAX_BLOCK_SIZE (currently 16384). Proof of concept is test torrent attached with piece size of 16383. reference ticket:4005#comment:2

2.84 (14309)

uint32_t
tr_getBlockSize (uint32_t pieceSize)
{
  uint32_t b = pieceSize;

  while (b > MAX_BLOCK_SIZE)
    b /= 2u;

  if (!b || (pieceSize % b)) /* not cleanly divisible */
    return 0;

  return b;
}

I didn't test what would happen when data actually begins downloading or if there would be any security issues. Improper piece sizes greater than MAX_BLOCK_SIZE result in "invalid or corrupt torrent file".

Attachments (2)

InvalidPieceSize.torrent (217 bytes) - added by cfpp2p 8 years ago.
InvalidPieceSize16383.zip (2.8 KB) - added by cfpp2p 8 years ago.
New test torrent with data file.

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by cfpp2p

comment:1 follow-up: Changed 8 years ago by cfpp2p

InvalidPieceSize16383.torrent correctly downloaded or uploaded. New test torrent with data file attached.

Changed 8 years ago by cfpp2p

New test torrent with data file.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by x190

Replying to cfpp2p:

InvalidPieceSize16383.torrent correctly downloaded or uploaded. New test torrent with data file attached.

So can "(pieceSize % b)) /* not cleanly divisible */" go?

comment:3 in reply to: ↑ 2 Changed 8 years ago by cfpp2p

Replying to x190:

So can "(pieceSize % b)) /* not cleanly divisible */" go?

/**
 * Decide on a block size. Constraints:
 * (1) most clients decline requests over 16 KiB
 * (2) pieceSize must be a multiple of block size
 */
uint32_t
tr_getBlockSize (uint32_t pieceSize)

The code seems to obey the constraints it issues.

A piece size of 16383 results in a block size of 16383. ( MAX_BLOCK_SIZE set to 16384 )

uint32_t b = pieceSize; while (b > MAX_BLOCK_SIZE) b /= 2u;

result: Block ( b ) size is not above 16KiB, obeys rule (1)

(pieceSize % b)

Piece size and block size are the same so there is no remainder. Piece size is a multiple of 1, of block size ( if that makes any sense... ), rule (2)

I've also tested that if piece size is any multiple of MAX_BLOCK_SIZE ( like 49152 for piece size ) that also works correctly for uploading and downloading. Transmission happily accepts, downloads and/or uploads torrents like this. The code in libtransmission looks to me to handle just what is stated in the rules. I didn't see anything in the code to indicate otherwise and testing confirmed this. There's nothing specific about powers of two, only multiples of MAX_BLOCK_SIZE.

libtransmission's code should work with practically any piece size. The deciding factor is that block(s) must fit perfectly even within a piece otherwise libtransmission's code won't work. The constraint is what clients will accept, and too big a block might be refused by clients. The processing code itself looks to handle any sort of piece size as long as the block size could be chosen to facilitate this.

Last edited 8 years ago by cfpp2p (previous) (diff)

comment:4 follow-up: Changed 8 years ago by x190

@cfpp2p Can you test the various odd pieceSize torrents (non-16KB cleanly divisible) documented in #4005 with the following code to see if any/all will work with various clients?

uint32_t
tr_getBlockSize (uint32_t pieceSize)
{
  uint32_t b = pieceSize;

  if (!b)
    return 0;

  while (b > MAX_BLOCK_SIZE && (b % 2u) == 0)
    b /= 2u;
 
  return b;
}
Last edited 8 years ago by x190 (previous) (diff)

comment:5 in reply to: ↑ 4 Changed 8 years ago by cfpp2p

Replying to x190:

@cfpp2p Can you test the various odd pieceSize torrents (non-16KB cleanly divisible) ...

The thing is both your code and the original code are not limited to 16KB cleanly divisible, but only multiples of MAX_BLOCK_SIZE ( as stated in comment:3 ) when piece size is greater than MAX_BLOCK_SIZE ( b > MAX_BLOCK_SIZE ).

When piece size is less than MAX_BLOCK_SIZE block size is set to piece size

( uint32_t b = pieceSize; )

Your line: while (b > MAX_BLOCK_SIZE && (b % 2u) == 0) does nothing to stop any piece size when initial block size b/pieceSize ( uint32_t b = pieceSize; ) is already equal to or smaller than MAX_BLOCK_SIZE. So does nothing different than the existing code. When b > MAX_BLOCK_SIZE and block size b is not cleanly divisible by two, your code could result in a block size not an even multiple of MAX_BLOCK_SIZE which will fail in the rest of the libtransmission code.

To be clear, the existing code works ( other than ticket:4005#comment:5 ) it's just whether we want to these types of piece sizes. Other clients I tested work correctly with transmission.

I can test more but I'm not sure what would be the point until a more defined direction for this ticket is decided upon.

comment:6 follow-up: Changed 8 years ago by x190

/**
 * Decide on a block size. Constraints:
 * (1) most clients decline requests over 16 KiB
 * (2) pieceSize must be a multiple of block size
 */

I was hoping to check the veracity of (1). I think comment:4 honors (2). Something would need to be done about MAX_BLOCK_SIZE in other parts of the code.

According to forum posters I have dealt with, at least some other clients manage to handle non-power-of-2 piece sizes larger than MAX_BLOCK_SIZE.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by cfpp2p

Replying to x190:

According to forum posters I have dealt with, at least some other clients manage to handle non-power-of-2 piece sizes larger than MAX_BLOCK_SIZE.

Transmission already does by my testing, as long as it's an exactly even multiple of MAX_BLOCK_SIZE ( currently 16384 ) with no remainder i.e. 49152, which is not a power of two.

Block size never exceeds MAX_BLOCK_SIZE as is within the transmission code. It's whether we want to continue allowing the different piece sizes, and I don't see any problems as long as ticket:4005#comment:5 is attended to.

You could change MAX_BLOCK_SIZE to something else to test the veracity of (1) but I don't see the point. Any block-size or piece-size less than 16384 works currently, and functions correctly by my testing. Transmission allows this as I explained in comment:3. The original thought on this ticket was just that, any piece size less than 16384 works. My question was do we want to allow this. The discovery of non-power-of-2 piece sizes larger than MAX_BLOCK_SIZE came later.

Last edited 8 years ago by cfpp2p (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 8 years ago by x190

Replying to cfpp2p:

Replying to x190:

According to forum posters I have dealt with, at least some other clients manage to handle non-power-of-2 piece sizes larger than MAX_BLOCK_SIZE.

Transmission already does by my testing, as long as it's an exactly even multiple of MAX_BLOCK_SIZE ( currently 16384 ) with no remainder i.e. 49152, which is not a power of two.

49152 and a handful of other specific examples only work because you can keep dividing by 2 without a remainder until you get to or below MAX_BLOCK_SIZE, so, assuming 16KiB is not written in stone, we are arbitrarily excluding other piece sizes greater than MAX_BLOCK_SIZE that don't fit this criterion. However, it may not be practical to increase MAX_BLOCK_SIZE. https://wiki.theory.org/BitTorrentSpecification

The original thought on this ticket was just that, any piece size less than 16384 works. My question was do we want to allow this. The discovery of non-power-of-2 piece sizes larger than MAX_BLOCK_SIZE came later.

Whether we continue to allow this would depend on not only if it works, but if it works without security, memory, or verification issues. MAX_BLOCK_SIZE is used in several places and I'm sure you understand the implications far better than I.

Last edited 8 years ago by x190 (previous) (diff)

comment:9 Changed 8 years ago by cfpp2p

closed as duplicate of ticket:5755

comment:10 Changed 8 years ago by cfpp2p

  • Resolution set to duplicate
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.