Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prenner/fix hashing images #1722

Merged
merged 3 commits into from
Dec 13, 2024
Merged

Conversation

prenner
Copy link
Contributor

@prenner prenner commented Dec 13, 2024

Summary

as detailed here, PNGs fail to hash when passed to the /h/hash endpoint. PNG hashes work via the threatexchange CLI because it is guaranteed that the contents are flushed before hashing occurs:

with tempfile.NamedTemporaryFile("wb", delete=False) as tmp:
    logging.debug("Writing -- to %s", tmp.name)
    tmp.write(resp.content)
filename = pathlib.Path(tmp.name)

but when filename is set in the with block, this is not the case for PNGs. manually call flush to fix this

https://discuss.python.org/t/should-tempfile-namedtemporaryfile-automatically-flush-the-buffer/17388 - in python 3.13 (we are on 3.11) there is a delete_on_close that we could use but deletion isn't guaranteed

Test Plan

Confirmed that the endpoint can now hash PNGs with flush added

@prenner prenner force-pushed the prenner/fix-hashing-images branch from c75248f to 20e8c7d Compare December 13, 2024 20:52
Comment on lines +58 to +59
with tmp.file as temp_file: # this ensures that bytes are flushed before hashing
temp_file.write(download_resp.content)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work too?

Suggested change
with tmp.file as temp_file: # this ensures that bytes are flushed before hashing
temp_file.write(download_resp.content)
tmp.write(download_resp.content)
tmp.flush()

@Dcallies Dcallies merged commit 0d4e771 into facebook:main Dec 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants