search

dlbeer / quirc

QR decoder library
Other
865 stars 285 forks source link

Fix possible overflows in otsu() #103

Closed yamt closed 3 years ago

yamt commented 3 years ago

Thanks for the PR @yamt

After this patch we correctly support images twice as big as before the patch, but we can still overflow unsigned int. An overflow is better after the patch since it's not UB anymore, we'll just ignore some (if not most) of the pixels.

Have you considered using size_t instead of unsigned int here? I'm asking because q->w * q->h < SIZE_MAX is an invariant is already ensured by quirc_resize(). It will bump the memory usage of histogram in all cases though, so I'm unsure if that's the right trade-off. cc @dlbeer

i have no strong opinions either ways. if it's controversial, i can drop the commit because what actually matters to me is another commit in this PR.

kaworu commented 3 years ago

@dlbeer could you please take a look?

dlbeer commented 3 years ago

This looks like a good idea, but I'd suggest using long long for sum instead of double. We do use floating-point quite a bit, but not on a per-pixel basis so far.

yamt commented 3 years ago

This looks like a good idea, but I'd suggest using long long for sum instead of double. We do use floating-point quite a bit, but not on a per-pixel basis so far.

it isn't a per-pixel basis. it's per-histgram-buckets, for which we already use double below.

dlbeer commented 3 years ago

Sorry, yes you're right. Looks fine in that case.