foliojs / fontkit

An advanced font engine for Node and the browser
1.49k stars 219 forks source link

Error subsetting CFF font with no Subrs #42

Open jahewson opened 8 years ago

jahewson commented 8 years ago

Attempting to subset a CFF font which does not contain a Subrs entry in its Private dictionary results in an exception in subsetSubrs. The problem code is here.

kblomster commented 7 years ago

I believe I ran into this when attempting to render a PDF with Futura PT using pdfmake. Is there any known workaround? I guess I could convert to TTF, but I'm unsure if the font license allows that.

/Users/kblomster/Code/redacted/api/node_modules/fontkit/index.js:13405
    for (var i = 0; i < subrs.length; i++) {
                             ^

TypeError: Cannot read property 'length' of null
    at CFFSubset.subsetSubrs (/Users/kblomster/Code/redacted/api/node_modules/fontkit/index.js:13405:30)
    at CFFSubset.createCIDFontdict (/Users/kblomster/Code/redacted/api/node_modules/fontkit/index.js:13498:30)
    at CFFSubset.encode (/Users/kblomster/Code/redacted/api/node_modules/fontkit/index.js:13548:12)
    at /Users/kblomster/Code/redacted/api/node_modules/fontkit/index.js:13027:13
    at _combinedTickCallback (internal/process/next_tick.js:73:7)
    at process._tickCallback (internal/process/next_tick.js:104:9)
kblomster commented 7 years ago

I tried adding a simple if (!subrs) return res; in CFFSubset.js after line 35, and that appears to "work" in that it prevents the error above and appears to render my PDF, but I have absolutely no idea if this is actually a viable solution or not. Maybe it helps someone else?

Pomax commented 7 years ago

another option is to change the function signature to have a default argument that causes the for loop to immediately exit:

subsetSubrs(subrs = [], used) {
  ...
}

This introduces less code, but also avoids an early return and early returns are great for ensuring code is fast (in that vein, an early return if (!subrs) return []; rather than allocating a res symbol would be even faster)

kblomster commented 7 years ago

If that solution doesn't introduce problems, I can send a pull request for it. After looking at the code I think it won't break things, since the requested subset does not necessarily need to use any subrs anyway (I think?), but I certainly can't claim to understand the actual mechanics of subsetting a CFF font in any detail.

Getting this fixed would be nice for me.

jahewson commented 7 years ago

The original issue which I opened against the old coffee script source no longer exists, as there is now a check for Subrs in subsetFontdict().

However, the corresponding line in createCIDFontdict() is missing this check and that seems to be the cause of your issue, looking at the stack trace. Presumably that's because you have a CID font, while I had a Type 1-equivalent font.

The advantage of adding the missing null check vs. the proposed fix above is that it won't unnecessarily create an empty Subrs dictionary in the subset font. No Subrs dictionary is needed at all.