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

Make hb_sanitize_context_t::max_ops unsigned #2079

Closed
wants to merge 1 commit into from

Conversation

zauguin
Copy link
Contributor

@zauguin zauguin commented Dec 16, 2019

Currently max_ops in hb_sanitize_context_t is signed, but in ...::start_processing an unsigned value is assigned:

    this->max_ops = hb_max ((unsigned int) (this->end - this->start) * HB_SANITIZE_MAX_OPS_FACTOR,
			 (unsigned) HB_SANITIZE_MAX_OPS_MIN);

If this->end - this->start is big enough, then (this->end - this->start) * HB_SANITIZE_MAX_OPS_FACTOR can not be represented in a signed int and the assignment overflows, leading to negative values in max_ops. Of course, then a font will no longer load correctly. There are at least two possible fixes for this: Just make max_ops unsigned (negative values don't make sense there anyway and the comparisons with 0 are written such that they still work with unsigned values) or use hb_min((unsigned) HB_SANITIVE_MAX_OPS_MAX, ...) before assigning to ->max_ops to provide an upper bound for the value.

I used the first approach here because I wasn't sure if max_ops should be limited to HB_SANITIVE_MAX_OPS_MAX or if that constant only has meaning for special fonts.

The original issue had appeared in latex3/luaotfload#126 but can be reproduced without LuaTeX:

Download SourceHanNotoCJK.ttc from https://github.com/adobe-fonts/source-han-super-otc/releases (~390MiB) and try

hb-shape --font-file /path/to/SourceHanNotoCJK.ttc --face-index 17 "中國哲學書電子化計劃"

On my system this gives only a collection of GID 0 glyphs:

[gid0=0+1000|gid0=1+1000|gid0=2+1000|gid0=3+1000|gid0=4+1000|gid0=5+1000|gid0=6+1000|gid0=7+1000|gid0=8+1000|gid0=9+1000]

the same font in hb-view gives hb-view: FT_New_Memory_Face fail.

Avoids underflows when assigning unsigned values
@behdad
Copy link
Member

behdad commented Dec 17, 2019

It's signed such that if we intentionally or accidentally decrement it multiple times before checking, it doesn't just wrap around. So, I want to keep it that way.

If font actually overflows that, breaking the font is also a good idea IMO. Anyway, just saturate it instead of overflow sounds best to me.

@zauguin
Copy link
Contributor Author

zauguin commented Dec 17, 2019

@behdad OK, I added a PR for the hb_min((unsigned) HB_SANITIVE_MAX_OPS_MAX, ...) approach as #2080 then.

@behdad
Copy link
Member

behdad commented Dec 17, 2019

Thanks.

@behdad behdad closed this Dec 17, 2019
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.

None yet

2 participants