[tx] fix segfaults due to old function call #1649
Conversation
|
I think we've discussed this before already, but we definitely need to make these fatals vs fprintf vs returning error code ways of error handling more consistent. Let me know if you have thoughts for the ones I did in this PR, and we can also discuss the overall usage of these various ways in our next team meeting! |
| if (fileName == NULL) { | ||
| fatal(h, ufoErrParse, "Encountered glyph reference in contents.plist with an empty file path. Text: '%s'.", getBufferContextPtr(h)); | ||
| } |
There was a problem hiding this comment.
I deleted this after trying to reach this with various test cases — I believe it is unreachable. addNewGLIFRec is called from two functions: parseCIDMap and addGLIFRec. When called from parseCIDMap, this should not be reachable since we do not proceed to these lines of code if parsing the CIDMap. When called from addGLIFRec, since it already handles this before calling addNewGLIFRec, it is already handled by the time these lines are reached. This is why I thought this was repetitive and unnecessary.
| if (h->stm.src == NULL) { | ||
| fprintf(stderr, "Failed to open glif file in parseGLIF: %s.\n", glifRec->glifFilePath); | ||
| return ufoErrSrcStream; | ||
| } | ||
| if (h->cb.stm.seek(&h->cb.stm, h->stm.src, 0)) { | ||
| fprintf(stderr, "Failed to open glif file in parseGLIF: %s.\n", glifRec->glifFilePath); | ||
| return ufoErrSrcStream; |
There was a problem hiding this comment.
I condensed these fatals into one if statement, but truthfully I'm not sure what the difference between these two are. I understand the first one (my test case is a missing glif file), but not sure if/how I can reach the second line.
| if (gi == NULL) { | ||
| message(h, "Missing component base attribute. Glyph: %s, Context: %s.\n", glifRec->glifFilePath, getBufferContextPtr(h)); | ||
| result = ufoErrNoGlyph; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
I was unsure about whether or not I should remove this for repetitiveness. I experimented with ways to getting to these lines, but they all were caught by line 2611 (in the updated file). Looking further into it, this function is called within parseGLIF, while line 2611 is called while within preParseGLIF. Since preParseGLIF is before parseGLIF, this should technically be dealt with already (as my test cases showed). However, I'm still unsure. I can leave it in while removing the getBufferContextPtr, but I don't have an idea for how to test it.
| fprintf(stderr, "Failed to open glif file in parseGLIFOutline. Glyph: %s. Context: %s\n.", glifRec->glyphName, getBufferContextPtr(h)); | ||
| fatal(h, ufoErrSrcStream, 0); |
There was a problem hiding this comment.
Reworded this so we're not referring to a function call in an error message as well as removed getBufferContextPtr
Logging is already quite different in my future-work branches. The less we change things in |
That was SO FAST that I couldn't even finish writing a response to Frank's message 😆 thank you! |
Description
Frank reported a bug where makeotf crashed with segfault due to an unexpected point type in a glif file. There was error handling already set up for this, but called a function getBufferContextPtr that was meant for the pre-libxml2 way of parsing tokens. I removed all calls of this function where the caller is using the libxml2 way of parsing and added some test cases.
Side note: There are some unused token parsing functions leftover in uforead that I would like to clean up in a separate PR. I partly kept them there in case they illustrated functions that we needed to convert to libxml2 — doesn't look like it from a quick glance, but I'll investigate this further.
Checklist: