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

lvrend: fix possible buffer overflow #613

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Jan 31, 2025

When trying to load of 34 MB EPUB (Kotlin Docs), the actual maximum length goes up to 6037.


This change is Reviewable

@benoit-pierre benoit-pierre force-pushed the pr/fix_possible_buffer_overflows branch 2 times, most recently from 3791b5b to 7a51eb6 Compare January 31, 2025 21:02
@benoit-pierre benoit-pierre force-pushed the pr/fix_possible_buffer_overflows branch from 7a51eb6 to a5e7eae Compare January 31, 2025 21:57
@poire-z
Copy link
Contributor

poire-z commented Jan 31, 2025

Not sure I like that, replacing the cheap static buffers with more expensive new alloc & deallocs each time we enter this section of the code, which may be called a lot with tables, floats, ruby...

There is the same kind of work in lvtextfm.cpp, with static buffers too, but there, the logic is we stop feeding and go lastFont->measureText() when we reach i >= start+MAX_TEXT_CHUNK_SIZE, so ther's no overflow (but possibly some issue if there's a ligature/clluster where we break?).

In my getRenderedWidths(), it looks like I don't do that, although the while (true) is there to handle incremental measureText(). So, this looks like a bug and a case I didn't think about.

I'd rather fix it in the spirit of the current code, than switching to new/delete.
Although in here (maybe in lvtextfm.cpp, dunno), there's no re-entrance, so the widths and flags could be created on the stack (without new/delete), which looks to me like it's cheaper than new/delete and may be cheaper in speed for access than a static created further away in memory ? Can we create dynamically sized arrays on the stack ?

(I remember I've seen in some changes of yours that you haven't pushed here that you were replacing all the static m_* buffers in lvtextfm with new/delete. I would have stated the avove if you had proposed it here :))

@benoit-pierre
Copy link
Contributor Author

There is the same kind of work in lvtextfm.cpp, with static buffers too, but there, the logic is we stop feeding and go lastFont->measureText() when we reach i >= start+MAX_TEXT_CHUNK_SIZE, so ther's no overflow (but possibly some issue if there's a ligature/clluster where we break?).

And I hate it too: it's wasting 680KB (on x86_64)! I've benchmarked the change on PC, and I'm not seeing any meaningful impact.

@poire-z
Copy link
Contributor

poire-z commented Jan 31, 2025

it's wasting 680KB

It's not wasted: it's re-used each time you turn a page to render new paragraphs. It's there, already allocated, waiting for your requests, without the neeed to malloc and free. (The same goes for the lvtextfm m_* buffers - it's not just used when we render() the full book, it's used each time we need to draw a paragraph - there are a small amount of rendered paragraphs that is cached, so when you go back a page you may meet them, but when you go forwards, new paragraphs needs to be renredered for drawing).
Even if you don't notice an impact, on paper and logically, not having to malloc and free these chunks should be less expensive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants