Opened 11 years ago

Closed 11 years ago

#3622 closed Bug (fixed)

verification ignores return value of pread

Reported by: benshu Owned by: charles
Priority: Normal Milestone: 2.12
Component: libtransmission Version: 2.10
Severity: Normal Keywords: verification
Cc:

Description

Whenever pread reads less bytes than expected

  • the SHA1 is not updated for the read data and
  • the pointers are advanced by the expected rather than the actual amount of bytes read.

Attachments (1)

fixVerify.patch (1.7 KB) - added by Longinus00 11 years ago.
potential fix

Download all attachments as: .zip

Change History (11)

Changed 11 years ago by Longinus00

potential fix

comment:1 Changed 11 years ago by charles

  • Milestone changed from None Set to 2.11
  • Status changed from new to assigned

comment:2 Changed 11 years ago by charles

@Longinus00: nice catch on pieceBytesRead, too :)

comment:3 Changed 11 years ago by charles

@Longinus00:

  1. The tr_tordbg message has a couple of errors in it (memory leak, wrong integral types, and reliance on tr_torrentFindFile() succeeding) but since your comment indicates it's a placeholder, I'm not worried about it at this point.
  1. More importantly, pread() returns zero on EOF. We calculate leftInFile from (tr_file.length - filepos) but what happens if we're not using full preallocation? In the old code we continue to march ahead anyway, so leftInFile is decremented. In fixVerify.patch we'll hit EOF, set bytesThisPass to 0, decrement leftInFile by 0, and get stuck in an infinite loop. If you want to do it this way, you'll need to replace tr_file.length with a stat() call to get the file's true size, and also add code to handle the missing bytes at the end of a file that hasn't been fully allocated.

comment:4 Changed 11 years ago by charles

Hm, I'm wrong about point 2...

comment:5 Changed 11 years ago by Longinus00

Replying to charles:

@Longinus00:

  1. The tr_tordbg message has a couple of errors in it (memory leak, wrong integral types, and reliance on tr_torrentFindFile() succeeding) but since your comment indicates it's a placeholder, I'm not worried about it at this point.
  1. More importantly, pread() returns zero on EOF. We calculate leftInFile from (tr_file.length - filepos) but what happens if we're not using full preallocation? In the old code we continue to march ahead anyway, so leftInFile is decremented. In fixVerify.patch we'll hit EOF, set bytesThisPass to 0, decrement leftInFile by 0, and get stuck in an infinite loop. If you want to do it this way, you'll need to replace tr_file.length with a stat() call to get the file's true size, and also add code to handle the missing bytes at the end of a file that hasn't been fully allocated.
  1. Yea, that was just so I could confirm that this was the correct fix for benshu's verification problem. It prints lots of false positives for truncated files as per your comment 2.
  1. That bug existed for the few minutes before I uploaded a fixed version. ;)
Last edited 11 years ago by Longinus00 (previous) (diff)

comment:6 Changed 11 years ago by charles

  • Milestone changed from 2.11 to Sometime

Bumping this from "2.11" to "Sometime" because the dev team has agreed on a quick release cycle for 2.11 and I'd feel better getting more testing on this patch before putting it in a release. 2.12 maybe?

comment:7 Changed 11 years ago by charles

Maybe I'm being stupid, but after rereading this ticket... is this actually a problem at all? If pread() fails, the checksum for that piece will fail no matter what we do with pread()'s return value. What benefit does adding this error handling really add?

Is this just for the case where pread() can only read in smaller chunks than what we request? If so, is that behavior that actually occurs in the real world?

comment:8 Changed 11 years ago by Longinus00

Pread failing would return -1, if it works it returns the number of bytes read (which is not necessarily equal to the number requested). Even though it is unlikely transmission shouldn't ignore the return value of pread.

comment:9 Changed 11 years ago by charles

  • Milestone changed from Sometime to 2.12

committed fixVerify.patch to trunk in r11339

comment:10 Changed 11 years ago by charles

  • Resolution set to fixed
  • Status changed from assigned to closed

Seems to be working alright; no complaints yet. Closing the ticket...

Note: See TracTickets for help on using tickets.