Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Lib/hashlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,8 @@ def file_digest(fileobj, digest, /, *, _bufsize=2**18):

if hasattr(fileobj, "getbuffer"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duck typing this is just checking "does it have a getbuffer" method. I could see getbuffer implementations which only return the data which will show up in read vs. what BytesIO does (the whole allocated data). I don't think there is a formalized "what does getbuffer do" with this case covered.

For new feature 3.16+ I think this approach is reasonable / if people have other getbuffer implementations they'll let us know. As a bugfix backport it seems like a subtle breaking change to performance sensitive code. To backport I think should explicitly check isinstance(fileobj, io.BytesIO).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can it be a breaking change? it was already like that for the past years

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And fileobj.getbuffer() returns a view on the buffer and buf[fileobj.tell():] returns a view as well, so 0-copy is still ensure, or am I missing something?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR, we document file_digest as follows:

fileobj must be a file-like object opened for reading in binary mode. It accepts file objects from builtin open(), BytesIO instances, SocketIO objects from socket.socket.makefile(), and similar. fileobj must be opened in blocking mode, otherwise a BlockingIOError may be raised.

So if people are doing something else, they're on their own.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context: the file_digest code was shipped in 3.11 and has had few changes since: https://github.com/python/cpython/pull/31930/changes.

While the comment asserts "this is a BytesIO" custom I/O stacks for Python exist. Searching there are quite a few custom getbuffer implementations outside of io.BytesIO (Many for display buffers, but some I/O buffers). Those pass this code and the typeshed stub protocol (https://github.com/python/typeshed/blob/6875aaf17c8322b00499f79276340fec4cc87451/stdlib/hashlib.pyi#L97-L109) which just assert "getbuffer method required". Custom optimized I/O stacks would likely implement a getbuffer which produces the result the codebase cares about. That might pay attention to the file offset, dropping already read bytes or might contain all bytes ever written.

The change here:

  1. Makes it so in addition to getbuffer() the object must support a tell() method unconditionally
  2. Currently implements that the file offset / tell result is required to go from "all data" to the "data not yet read".

Neither of those are "required" to me from the typeshed protocol or duck typing. The added tell requirement would break code which runs fine today and so is a breaking change.

If we "narrow" this to just BytesIO explicitly then requiring tell isn't too big of a hurdle and to me it does fix what feels like unintended behavior. Adding a requirement for tell is changing the API meaningfully hence 3.16+.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what you mean now. Ok this makes sense. I will use an isinstance check for the fast path in all versions and let the slow path orherwise. Custom implementations without tell() would fall back to the slow path.

# io.BytesIO object, use zero-copy buffer
digestobj.update(fileobj.getbuffer())
with fileobj.getbuffer() as buf:
digestobj.update(buf[fileobj.tell():])
return digestobj

# Only binary files implement readinto().
Expand Down
72 changes: 44 additions & 28 deletions Lib/test/test_hashlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import itertools
import logging
import os
import random
import re
import sys
import sysconfig
Expand Down Expand Up @@ -102,6 +103,20 @@ def read_vectors(hash_name):
)


def make_hash_objects(*digestmods, buf=b"", **kwargs):
objects = []
for digestmod in digestmods:
try:
if callable(digestmod):
obj = digestmod(buf)
else:
obj = hashlib.new(digestmod, buf, **kwargs)
except ValueError:
continue
objects.append((digestmod, obj))
return objects


class HashLibTestCase(unittest.TestCase):
supported_hash_names = ( 'md5', 'MD5', 'sha1', 'SHA1',
'sha224', 'SHA224', 'sha256', 'SHA256',
Expand Down Expand Up @@ -524,7 +539,8 @@ def test_blake2_update_over_4gb(self):
self.assertEqual(h.hexdigest(), "8a268e83dd30528bc0907fa2008c91de8f090a0b6e0e60a5ff0d999d8485526f")

def check(self, name, data, hexdigest, shake=False, **kwargs):
length = len(hexdigest)//2
n = len(data)
length = len(hexdigest) // 2
hexdigest = hexdigest.lower()
constructors = self.constructors_to_test[name]
# 2 is for hashlib.name(...) and hashlib.new(name, ...)
Expand All @@ -533,47 +549,47 @@ def check(self, name, data, hexdigest, shake=False, **kwargs):
m = hash_object_constructor(data, **kwargs)
computed = m.hexdigest() if not shake else m.hexdigest(length)
self.assertEqual(
computed, hexdigest,
"Hash algorithm %s constructed using %s returned hexdigest"
" %r for %d byte input data that should have hashed to %r."
% (name, hash_object_constructor,
computed, len(data), hexdigest))
computed, hexdigest,
"Hash algorithm %s constructed using %s returned hexdigest"
" %r for %d byte input data that should have hashed to %r."
% (name, hash_object_constructor, computed, n, hexdigest)
)
computed = m.digest() if not shake else m.digest(length)
digest = bytes.fromhex(hexdigest)
self.assertEqual(computed, digest)
if not shake:
self.assertEqual(len(digest), m.digest_size)

def generate_sub_hexdigest(pos=-1):
if pos < 0:
pos = 0 if n == 0 else random.randrange(0, n)
pos += 1 # ensure pos is at least 1 (allowed to be out of range)
buf = data[pos:]
objects = make_hash_objects(name, *constructors, buf=buf, **kwargs)
hexdigests = {obj.hexdigest() for _, obj in objects}
self.assertEqual(len(hexdigests), 1, f"bad digests: {objects}")
return pos, hexdigests.pop()

if not shake and kwargs.get("key") is None:
# skip shake and blake2 extended parameter tests
self.check_file_digest(name, data, hexdigest)
for pos in sorted({-1, 1, n - 1, n, n + 1}):
with self.subTest(pos=pos):
pos, hexdigest2 = generate_sub_hexdigest(pos=pos)
self.check_file_digest(name, data, hexdigest2, pos)

def check_file_digest(self, name, data, hexdigest):
def check_file_digest(self, name, data, hexdigest, pos=0):
hexdigest = hexdigest.lower()
digests = []
for digest in [name, *self.constructors_to_test[name]]:
try:
if callable(digest):
digest(b"")
else:
hashlib.new(digest)
except ValueError:
# skip, algorithm is blocked by security policy.
continue
digests.append(digest)

digests = make_hash_objects(name, *self.constructors_to_test[name])
with tempfile.TemporaryFile() as f:
f.write(data)
buf = io.BytesIO(data)

for digest in digests:
buf = io.BytesIO(data)
buf.seek(0)
self.assertEqual(
hashlib.file_digest(buf, digest).hexdigest(), hexdigest
)
f.seek(0)
digestobj = hashlib.file_digest(f, digest)
self.assertEqual(digestobj.hexdigest(), hexdigest)
for digest, _ in digests:
for fobj in [buf, f]:
fobj.seek(pos)
digestobj = hashlib.file_digest(fobj, digest)
self.assertEqual(digestobj.hexdigest(), hexdigest)

def check_no_unicode(self, algorithm_name):
# Unicode objects are not allowed as input.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Ensure that :func:`hashlib.file_digest` honors the current position of the
file object when given an :class:`io.BytesIO` object. Patch by Bénédikt
Tran.
Loading