FIX: Handles invalid TCK files, prevents infinite loop

[anibalsolon]

Proper check for the magic number and the header's end, stopping the search at EOF.

Resolves #1139

[codecov]

Codecov Report

Base: 91.80% // Head: 91.81% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (a4c420d) compared to base (feb2063).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1140      +/-   ##
==========================================
+ Coverage   91.80%   91.81%   +0.01%     
==========================================
  Files         101      101              
  Lines       12453    12469      +16     
  Branches     2432     2239     -193     
==========================================
+ Hits        11433    11449      +16     
  Misses        693      693              
  Partials      327      327              

Impacted Files	Coverage Δ
nibabel/streamlines/tck.py	99.50% <100.00%> (+0.04%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[effigies]

Thanks @anibalsolon! This looks perfectly sensible. Two thoughts:

1. The Opener class is intended to allow us to ignore whether we were passed a filename or an open filehandle and act like a file handle. We should never need to access the fobj instance variable and instead just use the opener directly, such as opener.read() or for line in opener:. If there are methods that you need on it, we should add them, but I think everything you use should be available.

2. The use of asstr() seems like a code smell, as it indicates we're unsure whether we're getting text or binary data. It does seem worth figuring out how/why this is ever passed a file object in text mode and see if we can make sure that it's always 'rb'.

The first would be good to address here. The second can wait for another PR unless you're feeling really eager to explore this.

[effigies]

Looks good. Thanks!

[anibalsolon]

@effigies Hey! Sorry, I started answering here and got distracted. Thank you for the review, good one about the .fobj

For 2, the TCK file should be in binary mode, given that the data will be binary. I am working on a fix for that.

Regarding this text/binary mode guarantee, when providing a file object for Opener, would it be beneficial to make sure the file object is in the same mode as the requested for Opener? e.g.

fd = open('text.txt', 'r')

with Opener(fd):  # raise exception, default is 'rb'
    ...

with Opener(fd, 'w'):  # raise exception
    ...

with Opener(fd, 'r'):  # all good
    ...

Given the file object is provided directly to the Opener in some interfaces/formats (TCK and TRK), we do not know what the mode the fd is in. Or would it be better to do the checking in another layer of the code? I usually prefer to do these checkings instead of letting a random exception explode in the user's face, but not sure what's the philosophy for nibabel.

[effigies]

Ah, sorry for jumping the gun.

I agree it would make sense for the opener to raise an exception for a mode mismatch, but I'm not sure if we should start enforcing it when there is no explicit mode. In any event, this would be an API change. So in the short term:

fname = 'file'
fd = open('file', 'r')

assert Opener(fname).mode == 'rb'
assert Opener(fname, mode='r').mode == 'r'
assert Opener(fd).mode == 'r'
assert Opener(fd, mode='r').mode == 'r'

with assert_raises(IOError):
    Opener(fd, 'rb')

with assert_raises(IOError):
    Opener(fd, 'w')

If we do want to make Opener(open(fname, 'r')) raise the same error as Opener(open(fname, 'r'), mode='rb'), we'll need to do a DeprecationWarning cycle. Which would be the next release +2 major versions (7.0).