fix(koptocr): crash bugs from assert-only checks and uninitialized variables - #67
fix(koptocr): crash bugs from assert-only checks and uninitialized variables#67tachibana-shin wants to merge 6 commits into
Conversation
assert() is compiled out in release builds (NDEBUG), making bounds checks and bpp validation no-ops in production. Replace with proper runtime checks that work in all build configurations.
If box_type is neither 0 nor 1, pboxa and pnai remain uninitialized, leading to undefined behavior when dereferenced. Add else branch to return early on invalid input.
_k2settings and initstr are re-initialized on every call via k2pdfopt_settings_init_from_koptcontext() and k2pdfopt_settings_new_source_document_init(). The static qualifier provides no benefit, prevents proper re-initialization, and is not thread-safe.
Validate src pointer, data pointer, coordinates and dimensions before accessing bitmap memory. Returns NULL on invalid input instead of causing out-of-bounds reads.
| if (*pboxa == NULL && *pnai == NULL && src->bpp) { | ||
| assert(x + w <= src->width); | ||
| assert(y + h <= src->height); | ||
| if (x < 0 || y < 0 || x + w > src->width || y + h > src->height) |
There was a problem hiding this comment.
To clarify, the way I see it assert() is so you know immediately during development, while silently swallowing or working around any issues is generally the preferred behavior for a release build. The checks may be very similar but the purpose behind them is different. So I'd add rather than replace.
There was a problem hiding this comment.
Personally, I don't like using assert in these places at all, but I'll add them back in
There was a problem hiding this comment.
I also want to keep any diffs as minimal as possible because these are custom patches on top of a code base. ;-)
There was a problem hiding this comment.
Pardon, this is our file. Either way, I like hard fails. Silently swallowing them means I don't know about it.
There was a problem hiding this comment.
Yes, please! I have some changes I need to PR, disabling dead code (among other things), maybe hold off further PRs until those have been merged, @tachibana-shin, no sense in duplicating work or fixing stuff we don't use.
There was a problem hiding this comment.
Yes, please! I have some changes I need to PR, disabling dead code (among other things), maybe hold off further PRs until those have been merged, @tachibana-shin, no sense in duplicating work or fixing stuff we don't use.
Great, I'm having a lot of trouble with some issues that I later discovered Koreader never even mentions.
There was a problem hiding this comment.
Pardon, this is our file. Either way, I like hard fails. Silently swallowing them means I don't know about it.
I've re-added the asserts here, but what about below—do you think we should re-add them there too? If we do, it would look something like this:
} else {
asserts(false);
}Add assertions to validate dimensions before processing.
| } | ||
| } else { | ||
| assert(src->bpp == 24); | ||
| } else if (src->bpp == 24) { |
There was a problem hiding this comment.
@Frenzie Do I need to add an assert here? It would look like this: } else { assert(false); }
There was a problem hiding this comment.
Looks like @benoit-pierre added that assert (not at my request) so I'll hand off that question.
3 participants
This change is