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.

Sign up for GitHub

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

Jump to bottom

[tx] fix segfaults due to old function call #1649

Merged
merged 4 commits into from
May 5, 2023
Merged

[tx] fix segfaults due to old function call #1649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d3a265c
[tx_test] add test and data
kaydeearts May 5, 2023
d8d41a2
[uforead] remove getBufferContextPtr from used fatals
kaydeearts May 5, 2023
4da8548
[tx_tests] move the formerly new tests to the glif parsing tests
kaydeearts May 5, 2023
c885603
[uforead] remove another getBufferontextPtr call and reduce repetitiv…
kaydeearts May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 6 additions & 22 deletions c/shared/source/uforead/uforead.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,9 +1187,6 @@ static GLIF_Rec* addNewGLIFRec(ufoCtx h, char* glyphName, xmlNodePtr cur) {
newGLIFRec->glyphName = glyphName;
newGLIFRec->glyphOrder = glyphOrder;
if (!h->parseState.CIDMap) { /* do not proceed if currently parsing CIDMap in lib.plist. This information will be filled in later. */
if (fileName == NULL) {
fatal(h, ufoErrParse, "Encountered glyph reference in contents.plist with an empty file path. Text: '%s'.", getBufferContextPtr(h));
}
Comment on lines -1190 to -1192
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

newGLIFRec->glifFileName = memNew(h, 1 + strlen(fileName));
sprintf(newGLIFRec->glifFileName, "%s", fileName);
newGLIFRec->altLayerGlifFileName = NULL;
Expand All @@ -1203,7 +1200,7 @@ static void addGLIFRec(ufoCtx h, char* glyphName, xmlNodePtr cur) {
GLIF_Rec* foundGlyph = NULL;
char* fileName = parseXMLKeyValue(h, cur);
if (fileName == NULL) {
fatal(h, ufoErrParse, "Encountered glyph reference in contents.plist with an empty file path. Text: '%s'.", getBufferContextPtr(h));
fatal(h, ufoErrParse, "Encountered glyph reference '%s' in contents.plist with an empty file path.", glyphName);
}
foundGlyph = findGLIFRecByName(h, glyphName);
if (foundGlyph == NULL){
Expand Down Expand Up @@ -1491,13 +1488,8 @@ static int preParseGLIF(ufoCtx h, GLIF_Rec* glifRec, int tag) {
h->cb.stm.clientFileName = glifRec->glifFilePath;
h->stm.src = h->cb.stm.open(&h->cb.stm, UFO_SRC_STREAM_ID, 0);
}
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;
Comment on lines -1494 to -1500
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ((h->stm.src == NULL) || (h->cb.stm.seek(&h->cb.stm, h->stm.src, 0))) {
fatal(h, ufoErrSrcStream, "Failed to open the %s glif file.\n", glifRec->glifFilePath);
}

dnaSET_CNT(h->valueArray, 0);
Expand Down Expand Up @@ -2508,7 +2500,7 @@ static int parseXMLPoint(ufoCtx h, xmlNodePtr cur, abfGlyphCallbacks* glyph_cb,
else if (strEqual(strType, "offcurve"))
continue; // type is already set to 0. x and y will get pushed on the stack, and no other operation will happen.
else {
fatal(h, ufoErrParse, "Encountered unsupported point type '%s' in glyph '%s'. Context: %s.\n", strType, glifRec->glyphName, getBufferContextPtr(h));
fatal(h, ufoErrParse, "Encountered unsupported point type '%s' in glyph '%s'.\n", strType, glifRec->glyphName);
result = ufoErrParse;
break;
}
Expand All @@ -2524,11 +2516,6 @@ static int parseGLIF(ufoCtx h, abfGlyphInfo* gi, abfGlyphCallbacks* glyph_cb, Tr

static int setParseXMLComponentValue(ufoCtx h, abfGlyphInfo* gi, abfGlyphCallbacks* glyph_cb, GLIF_Rec* glifRec, Transform* transform, Transform localTransform, Transform *newTransform, int result) {
Transform concatTransform;
if (gi == NULL) {
message(h, "Missing component base attribute. Glyph: %s, Context: %s.\n", glifRec->glifFilePath, getBufferContextPtr(h));
result = ufoErrNoGlyph;
return result;
}
Comment on lines -2527 to -2531
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/* Figure out transforms */
if (transform == NULL) {
Expand Down Expand Up @@ -2621,9 +2608,7 @@ static int parseXMLComponent(ufoCtx h, xmlNodePtr cur, GLIF_Rec* glifRec, abfGly
char* glyphName = getXmlAttrValue(attr);
if (!ctuLookup(glyphName, h->chars.byName.array, h->chars.byName.cnt,
sizeof(h->chars.byName.array[0]), postMatchChar, &index, h)) {
fprintf(stderr, "Could not find component base glyph %s. parent Glyph: %s\n", glyphName, glifRec->glifFilePath);
result = ufoErrNoGlyph;
break;
fatal(h, ufoErrNoGlyph, "Encountered glyph component reference '%s' with an empty file path.", glifRec->glifFilePath);
}
gi = &h->chars.index.array[h->chars.byName.array[index]];
} else {
Expand Down Expand Up @@ -3067,8 +3052,7 @@ static int parseGLIF(ufoCtx h, abfGlyphInfo* gi, abfGlyphCallbacks* glyph_cb, Tr
h->stm.src = h->cb.stm.open(&h->cb.stm, UFO_SRC_STREAM_ID, 0);

if (h->stm.src == NULL || h->cb.stm.seek(&h->cb.stm, h->stm.src, gi->sup.begin)) {
fprintf(stderr, "Failed to open glif file in parseGLIFOutline. Glyph: %s. Context: %s\n.", glifRec->glyphName, getBufferContextPtr(h));
fatal(h, ufoErrSrcStream, 0);
Comment on lines -3070 to -3071
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded this so we're not referring to a function call in an error message as well as removed getBufferContextPtr

fatal(h, ufoErrSrcStream, "Failed to open the %s glif file.\n", glifRec->glyphName);
}

h->stack.cnt = 0;
Expand Down
165 changes: 165 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-component-filepath.ufo/fontinfo.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>ascender</key>
<integer>730</integer>
<key>capHeight</key>
<integer>670</integer>
<key>copyright</key>
<string>Copyright 2014 - 2017 Adobe Systems Incorporated (http://www.adobe.com/), with Reserved Font Name 'Source'.</string>
<key>descender</key>
<integer>-240</integer>
<key>familyName</key>
<string>Source Serif Variable</string>
<key>guidelines</key>
<array>
</array>
<key>italicAngle</key>
<integer>0</integer>
<key>openTypeHheaAscender</key>
<integer>1036</integer>
<key>openTypeHheaDescender</key>
<integer>-335</integer>
<key>openTypeHheaLineGap</key>
<integer>0</integer>
<key>openTypeNameDesigner</key>
<string>Frank Grießhammer</string>
<key>openTypeNameLicense</key>
<string>This Font Software is licensed under the SIL Open Font License, Version 1.1. This license is available with a FAQ at: http://scripts.sil.org/OFL. This Font Software is distributed on an 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the SIL Open Font License for the specific language, permissions and limitations governing your use of this Font Software.</string>
<key>openTypeNameLicenseURL</key>
<string>http://scripts.sil.org/OFL</string>
<key>openTypeNameManufacturer</key>
<string>Adobe Systems Incorporated</string>
<key>openTypeNameManufacturerURL</key>
<string>http://www.adobe.com/type</string>
<key>openTypeOS2CodePageRanges</key>
<array>
<integer>0</integer>
<integer>1</integer>
<integer>2</integer>
<integer>3</integer>
<integer>4</integer>
<integer>7</integer>
<integer>8</integer>
<integer>29</integer>
</array>
<key>openTypeOS2Panose</key>
<array>
<integer>2</integer>
<integer>4</integer>
<integer>6</integer>
<integer>3</integer>
<integer>5</integer>
<integer>4</integer>
<integer>5</integer>
<integer>2</integer>
<integer>2</integer>
<integer>4</integer>
</array>
<key>openTypeOS2TypoAscender</key>
<integer>730</integer>
<key>openTypeOS2TypoDescender</key>
<integer>-270</integer>
<key>openTypeOS2TypoLineGap</key>
<integer>0</integer>
<key>openTypeOS2UnicodeRanges</key>
<array>
<integer>0</integer>
<integer>1</integer>
<integer>2</integer>
<integer>7</integer>
<integer>9</integer>
<integer>29</integer>
<integer>32</integer>
<integer>33</integer>
<integer>57</integer>
</array>
<key>openTypeOS2VendorID</key>
<string>ADBO</string>
<key>openTypeOS2WinAscent</key>
<integer>1036</integer>
<key>openTypeOS2WinDescent</key>
<integer>335</integer>
<key>postscriptBlueFuzz</key>
<integer>0</integer>
<key>postscriptBlueScale</key>
<real>0.0375</real>
<key>postscriptBlueValues</key>
<array>
<integer>-15</integer>
<integer>0</integer>
<integer>474</integer>
<integer>487</integer>
<integer>527</integer>
<integer>540</integer>
<integer>550</integer>
<integer>563</integer>
<integer>647</integer>
<integer>660</integer>
<integer>670</integer>
<integer>685</integer>
<integer>730</integer>
<integer>750</integer>
</array>
<key>postscriptFamilyBlues</key>
<array>
<integer>-15</integer>
<integer>0</integer>
<integer>475</integer>
<integer>488</integer>
<integer>527</integer>
<integer>540</integer>
<integer>549</integer>
<integer>563</integer>
<integer>646</integer>
<integer>659</integer>
<integer>669</integer>
<integer>684</integer>
<integer>729</integer>
<integer>749</integer>
</array>
<key>postscriptFamilyOtherBlues</key>
<array>
<integer>-249</integer>
<integer>-239</integer>
</array>
<key>postscriptFontName</key>
<string>SourceSerifVariable-Roman</string>
<key>postscriptOtherBlues</key>
<array>
<integer>-250</integer>
<integer>-240</integer>
</array>
<key>postscriptStemSnapH</key>
<array>
<integer>55</integer>
<integer>40</integer>
</array>
<key>postscriptStemSnapV</key>
<array>
<integer>80</integer>
<integer>90</integer>
</array>
<key>postscriptUnderlinePosition</key>
<integer>-75</integer>
<key>postscriptUnderlineThickness</key>
<integer>50</integer>
<key>styleMapFamilyName</key>
<string>Source Serif Variable</string>
<key>styleMapStyleName</key>
<string>regular</string>
<key>styleName</key>
<string>Roman</string>
<key>trademark</key>
<string>Source is a trademark of Adobe Systems Incorporated in the United States and/or other countries.</string>
<key>unitsPerEm</key>
<integer>1000</integer>
<key>versionMajor</key>
<integer>1</integer>
<key>versionMinor</key>
<integer>0</integer>
<key>xHeight</key>
<integer>474</integer>
</dict>
</plist>
42 changes: 42 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-component-filepath.ufo/glyphs/C_.glif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?xml version="1.0" encoding="UTF-8"?>
<glyph name="C" format="2">
<unicode hex="0043"/>
<advance width="632"/>
<outline>
<contour>
<point x="390" y="35" type="curve" smooth="yes"/>
<point x="251" y="35"/>
<point x="153" y="140"/>
<point x="153" y="335" type="curve" smooth="yes"/>
<point x="153" y="530"/>
<point x="262" y="635"/>
<point x="397" y="635" type="curve" smooth="yes"/>
<point x="429" y="635"/>
<point x="465" y="627"/>
<point x="502" y="607" type="curve"/>
<point x="521" y="492" type="line"/>
<point x="582" y="492" type="line"/>
<point x="578" y="637" type="line"/>
<point x="510" y="676"/>
<point x="439" y="685"/>
<point x="385" y="685" type="curve" smooth="yes"/>
<point x="190" y="685"/>
<point x="48" y="535"/>
<point x="48" y="335" type="curve" smooth="yes"/>
<point x="48" y="131"/>
<point x="184" y="-15"/>
<point x="386" y="-15" type="curve" smooth="yes"/>
<point x="456" y="-15"/>
<point x="525" y="-1"/>
<point x="588" y="33" type="curve"/>
<point x="592" y="178" type="line"/>
<point x="531" y="178" type="line"/>
<point x="512" y="63" type="line"/>
<point x="471" y="43"/>
<point x="429" y="35"/>
</contour>
</outline>
<anchor name="aboveUC" x="371" y="691"/>
<anchor name="belowLC" x="378" y="-22"/>
<anchor name="baseLC" x="379" y="0"/>
</glyph>
8 changes: 8 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-component-filepath.ufo/glyphs/C_acute.glif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
<glyph name="Cacute" format="2">
<unicode hex="0106"/>
<advance width="632"/>
<outline>
<component base="C"/>
</outline>
</glyph>
10 changes: 10 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-component-filepath.ufo/glyphs/contents.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>C</key>
<string></string>
<key>Cacute</key>
<string>C_acute.glif</string>
</dict>
</plist>
10 changes: 10 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-component-filepath.ufo/layercontents.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<array>
<array>
<string>public.default</string>
<string>glyphs</string>
</array>
</array>
</plist>
11 changes: 11 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-component-filepath.ufo/lib.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>public.glyphOrder</key>
<array>
<string>C</string>
<string>Cacute</string>
</array>
</dict>
</plist>
10 changes: 10 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-component-filepath.ufo/metainfo.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>creator</key>
<string>org.robofab.ufoLib</string>
<key>formatVersion</key>
<integer>3</integer>
</dict>
</plist>
26 changes: 26 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-filepath-cid.ufo/fontinfo.plist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple Computer//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>postscriptFontName</key>
<string>SourceHanSansVF-Heavy</string>
<key>styleName</key>
<string>Heavy</string>
<key>familyName</key>
<string>Source Han Sans</string>
<key>versionMajor</key>
<integer>2</integer>
<key>versionMinor</key>
<integer>3</integer>
<key>trademark</key>
<string>Copyright 2014-2020 Adobe (http://www.adobe.com/), with Reserved Font Name 'Source'. Source is a trademark of Adobe in the United States and/or other countries.</string>
<key>unitsPerEm</key>
<integer>1000</integer>
<key>postscriptFullName</key>
<string>Source Han Sans Heavy</string>
<key>postscriptWeightName</key>
<string>Heavy</string>
<key>postscriptUnderlinePosition</key>
<integer>-150</integer>
</dict>
</plist>
32 changes: 32 additions & 0 deletions tests/tx_data/input/ufo-glifs-parsing/null-filepath-cid.ufo/glyphs/cid00000.glif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?xml version="1.0" encoding="UTF-8"?>
<glyph name="cid00000" format="1" >
<advance width="1000"/>
<outline>
<contour>
<point x="100" y="-120" type="line"/>
<point x="900" y="-120" type="line"/>
<point x="900" y="880" type="line"/>
<point x="100" y="880" type="line"/>
</contour>
<contour>
<point x="500" y="421" type="line"/>
<point x="182" y="830" type="line"/>
<point x="818" y="830" type="line"/>
</contour>
<contour>
<point x="532" y="380" type="line"/>
<point x="850" y="789" type="line"/>
<point x="850" y="-29" type="line"/>
</contour>
<contour>
<point x="182" y="-70" type="line"/>
<point x="500" y="339" type="line"/>
<point x="818" y="-70" type="line"/>
</contour>
<contour>
<point x="150" y="789" type="line"/>
<point x="468" y="380" type="line"/>
<point x="150" y="-29" type="line"/>
</contour>
</outline>
</glyph>
Loading