feat: Hard-limit original image byte size to 2 * max_bytes if recoding fails (#8296)#8300
feat: Hard-limit original image byte size to 2 * max_bytes if recoding fails (#8296)#8300iequidoo wants to merge 1 commit into
2 * max_bytes if recoding fails (#8296)#8300Conversation
iequidoo
force-pushed
the
iequidoo/hard-limit-orig-img-size
branch
from
c081082 to
f2a315e
Compare
| context, | ||
| "Cannot check/recode image, using original data: {err:#}.", | ||
| ); | ||
| // NB: If the image can't be decoded, but the user had chosen `Viewtype::File` |
There was a problem hiding this comment.
Maybe this should be documented even at the API level because this isn't just an implementation detail, but an observable behavior
There was a problem hiding this comment.
Documented this for check_or_recode_image() which is a public function. Not sure if we want to document it at the top API level.
| *no_exif_ref = exif.is_none(); | ||
| // `max_bytes` isn't a strict limit for non-avatars, still, we don't want to send a | ||
| // gigabyte if recoding fails. | ||
| *can_use_original_ref = exif.is_none() && nr_bytes <= u64::try_from(max_bytes)? * 2; |
There was a problem hiding this comment.
This value seems arbitrary, and probably needs to be discussed further. Anyhow, such limit needs to be documented at api level.
Also nit: this uses a safe try_from to convert max_bytes to u64, yet then uses unchecked multiplication, let's use saturating multiplication instead.
There was a problem hiding this comment.
This value seems arbitrary, and probably needs to be discussed further. Anyhow, such limit needs to be documented at api level.
Have added a constant to crate::constants and improved the docs there. Maybe we want to even document all this, say, at the JSON-RPC level, not sure.
Also nit: this uses a safe try_from to convert max_bytes to u64, yet then uses unchecked multiplication, let's use saturating multiplication instead.
It's unlikely that we're going to replace all multiplications in the code with saturating_mul(), we have quite a lot. max_bytes actually comes from the config and it can't exceed 500_000, so usual multiplication is safe here, but i've replaced it with saturating_mul() so that it doesn't look strange next to safe try_from().
There was a problem hiding this comment.
I think we should always just use non-panicking versions, even if it seems obvious as this saves us time spent on thinking "can this panic?" in the future ;p
There was a problem hiding this comment.
I agree, otherwise every time an integer overflow-related bug occurs (even if there's no panic in the release configuration), you need to go through all the code and check it, that's why at least we shouldn't use as. Unfortunately, we have usual "wrapping around" multiplications in many palces
iequidoo
force-pushed
the
iequidoo/hard-limit-orig-img-size
branch
2 times, most recently
from
59c6b2e to
3ca061f
Compare
…ing fails (#8296) `max_bytes` isn't a strict limit for non-avatars (note: it's 500 kB by default), still, we don't want to send a gigabyte if recoding fails. Also document why we downgrade to `Viewtype::File` if the image couldn't be decoded.
iequidoo
force-pushed
the
iequidoo/hard-limit-orig-img-size
branch
from
3ca061f to
637dbbb
Compare
2 participants
max_bytesisn't a strict limit for non-avatars (note: it's 500 kB by default), still, we don'twant to send a gigabyte if recoding fails.
Also document why we downgrade to
Viewtype::Fileif the image couldn't be decoded.This PR is to have a separate discussion because the current code looks to have this point missed. I remember there were discussions that we don't have any hard limit because the user may send an image and optimistically switch to another chat or even app expecting that the image will be sent somehow, but i think that if the user really cares, they should check the result, otherwise there may even be no network etc. But i'm not a UX expert. Anyway, the reasoning should be documented and some test added, even if we don't want to change the code. CC @r10s
Related to: #8296