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

destroy hb_face correctly #9

Merged
merged 1 commit into from
Jul 2, 2021
Merged

Conversation

alsotang
Copy link
Contributor

@alsotang alsotang commented Jul 2, 2021

  1. the font file you use is too small, and I think it's broken. I download the file from RuntimeError: memory access out of bounds #8 , and update it.
  2. face should be destroyed. You can prove it by log the input value
    const input = exports.hb_subset_input_create_or_fail();
    . This value should not change when input args are the same. And when it get bigger and bigger, the program would fatal. Further more, all alloced ptr should be consistent when input args are the same.

see also https://github.com/harfbuzz/harfbuzzjs/blob/c141d5872698d6f0d95c2853e0ee4540e688dab1/subset/test.cc#L47

  1. exports.memory.grow(2000) seems too big. I doubt whether there is such a big font file.

alsotang added a commit to alsotang/harfbuzzjs that referenced this pull request Jul 2, 2021
@papandreou
Copy link
Owner

Please fix/point out one thing per PR/issue, makes it much easier to get things landed quickly :)

2. face should be destroyed. You can prove it by log the input value

Great catch, let's get that landed.

the font file you use is too small, and I think it's broken. I download the file from RuntimeError: memory access out of bounds #8 , and update it.

If you look at the test it's used in, the point is that it's truncated so we exercise the error handling:

subset-font/test/index.js

Lines 203 to 208 in 150b9dd

describe('with a truncated font', function () {
before(async function () {
this.truncatedTtfFont = await readFile(
pathModule.resolve(__dirname, '..', 'testdata', 'FZZJ-ZSXKJW.ttf')
);
});

But I'm OK with checking in the correct file and just truncate it in the test.

exports.memory.grow(2000) seems too big. I doubt whether there is such a big font file.

See Munter/subfont#145

@papandreou papandreou merged commit ff245d5 into papandreou:master Jul 2, 2021
@papandreou
Copy link
Owner

Released in 1.3.2

ebraminio pushed a commit to harfbuzz/harfbuzzjs that referenced this pull request Jul 3, 2021
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