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

negative values cannot be stored to size_t #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

negative values cannot be stored to size_t #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b11e3c
Silf::findClassIndex,Segment::findClassIndex,TtfUtil::LocaLookup retu…
himajin100000 Apr 8, 2019
cdbccaf
bitwise negative of signed int can be negative
himajin100000 Apr 8, 2019
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
6 changes: 4 additions & 2 deletions src/GlyphCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ of the License or (at your option) any later version.
#include "inc/GlyphFace.h"
#include "inc/Endian.h"
#include "inc/bits.h"
#include <cassert>

using namespace graphite2;

Expand Down Expand Up @@ -357,8 +358,9 @@ const GlyphFace * GlyphCache::Loader::read_glyph(unsigned short glyphid, GlyphFa
if (_glyf)
{
int xMin, yMin, xMax, yMax;
size_t locidx = TtfUtil::LocaLookup(glyphid, _loca, _loca.size(), _head);
void *pGlyph = TtfUtil::GlyfLookup(_glyf, locidx, _glyf.size());
int locidx = TtfUtil::LocaLookup(glyphid, _loca, _loca.size(), _head);
assert(locidx >= 0);
void *pGlyph = TtfUtil::GlyfLookup(_glyf, static_cast<size_t>(locidx), _glyf.size());
Copy link

Choose a reason for hiding this comment

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

Depending on the necessity of having a value filled into that bbox further down in this method, I suggest something like
Glyph pGlyph = nullptr; / is there such a generic type to avoid all the casting? */
int locidx = ...LocaLookup...;
if (locidx >= 0) pGlyph = ...GlyfLookup...;
if (pGlyph && ...GlyfBox...) ...etc...

Copy link
Contributor

@mhosken mhosken Apr 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

LocaLookup returns -1 and -2 as sentinel error values which are tested for in GlyfLookup. The assert() would cause a crash on faulty fonts rather than handling things tidily. Since -1 and -2 are sentinels it was felt adequate that the type is size_t (given that -1 and -2 as size_t are just big positive numbers adequately represented by -1 and -2). This modification doesn't add anything. And the assert introduces a bug.


if (pGlyph && TtfUtil::GlyfBox(pGlyph, xMin, yMin, xMax, yMax))
{
Expand Down
2 changes: 1 addition & 1 deletion src/Silf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ uint16 Silf::findPseudo(uint32 uid) const
return 0;
}

uint16 Silf::findClassIndex(uint16 cid, uint16 gid) const
int Silf::findClassIndex(uint16 cid, uint16 gid) const
Copy link
Contributor

Choose a reason for hiding this comment

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

The same principle of sentinels applies here.

{
if (cid > m_nClass) return -1;

Expand Down
2 changes: 1 addition & 1 deletion src/TtfUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,7 +1211,7 @@ unsigned int CmapSubtable12NextCodepoint(const void *pCmap310, unsigned int nUni
Technically this method should return an unsigned long but it is unlikely the offset will
exceed 2^31.
----------------------------------------------------------------------------------------------*/
size_t LocaLookup(gid16 nGlyphId,
int LocaLookup(gid16 nGlyphId,
const void * pLoca, size_t lLocaSize,
const void * pHead) // throw (std::out_of_range)
{
Expand Down
2 changes: 1 addition & 1 deletion src/inc/Segment.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class Segment
void associateChars(int offset, size_t num);
void linkClusters(Slot *first, Slot *last);
uint16 getClassGlyph(uint16 cid, uint16 offset) const { return m_silf->getClassGlyph(cid, offset); }
uint16 findClassIndex(uint16 cid, uint16 gid) const { return m_silf->findClassIndex(cid, gid); }
int findClassIndex(uint16 cid, uint16 gid) const { return m_silf->findClassIndex(cid, gid); }
int addFeatures(const Features& feats) { m_feats.push_back(feats); return int(m_feats.size()) - 1; }
uint32 getFeature(int index, uint8 findex) const { const FeatureRef* pFR=m_face->theSill().theFeatureMap().featureRef(findex); if (!pFR) return 0; else return pFR->getFeatureVal(m_feats[index]); }
void setFeature(int index, uint8 findex, uint32 val) {
Expand Down
2 changes: 1 addition & 1 deletion src/inc/Silf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class Silf

bool readGraphite(const byte * const pSilf, size_t lSilf, Face &face, uint32 version);
bool runGraphite(Segment *seg, uint8 firstPass=0, uint8 lastPass=0, int dobidi = 0) const;
uint16 findClassIndex(uint16 cid, uint16 gid) const;
int findClassIndex(uint16 cid, uint16 gid) const;
uint16 getClassGlyph(uint16 cid, unsigned int index) const;
uint16 findPseudo(uint32 uid) const;
uint8 numUser() const { return m_aUser; }
Expand Down
2 changes: 1 addition & 1 deletion src/inc/TtfUtil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ class Tag
const void * pHhea, int & nLsb, unsigned int & nAdvWid);

////////////////////////////////// primitives for loca and glyf lookup
size_t LocaLookup(gid16 nGlyphId, const void * pLoca, size_t lLocaSize,
int LocaLookup(gid16 nGlyphId, const void * pLoca, size_t lLocaSize,
const void * pHead); // throw (std::out_of_range);
void * GlyfLookup(const void * pGlyf, size_t lGlyfOffset, size_t lTableLen);

Expand Down
2 changes: 1 addition & 1 deletion src/inc/bits.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ inline unsigned int bit_set_count(signed long long v)
template<typename T>
inline unsigned int bit_set_count(T v)
{
static size_t const ONES = ~0;
static size_t const ONES = static_cast<size_t>(~0);

v = v - ((v >> 1) & T(ONES/3)); // temp
v = (v & T(ONES/15*3)) + ((v >> 2) & T(ONES/15*3)); // temp
Expand Down
11 changes: 8 additions & 3 deletions src/inc/opcodes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ Mozilla Public License (http://mozilla.org/MPL) or the GNU General Public
License, as published by the Free Software Foundation, either version 2
of the License or (at your option) any later version.
*/

#include <cassert>

#pragma once
// This file will be pulled into and integrated into a machine implmentation
// DO NOT build directly and under no circumstances ever #include headers in
Expand Down Expand Up @@ -233,12 +236,13 @@ STARTOP(put_subs_8bit_obs)
const int slot_ref = int8(param[0]);
const unsigned int input_class = uint8(param[1]),
output_class = uint8(param[2]);
uint16 index;
int index;
slotref slot = slotat(slot_ref);
if (slot)
{
index = seg.findClassIndex(input_class, slot->gid());
is->setGlyph(&seg, seg.getClassGlyph(output_class, index));
asset(index >= 0);
is->setGlyph(&seg, seg.getClassGlyph(output_class, static_cast<uint16>(index)));
Copy link

Choose a reason for hiding this comment

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

The assert can be helpful, but only has effect in debug builds. Otherwise this forces a call to getClassGlyph with the index 31767 in this case. If the idea in the first place is to avoid calling setGlyph with an invalid glyph, then guarding the call with an "if (index > 0)" might be appropiate?

Copy link
Author

@himajin100000 himajin100000 Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

true, "if(index >= 0)" thingy would be more helpful, but I wasn't sure what I should do in error handling. Just returning without doing anything? showing a dialog that alerts user about the error? throwing an exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please stop slamming asserts in everywhere where you have just switched from size_t to int because you know it may well return something < 0! If you static cast from int to uint16 what's so bad about having the cast occur earlier?
Graphite does not throw exceptions. That is a design constraint to allow it to have a pure C interface and not be dependent on libstdc++.

}
ENDOP

Expand Down Expand Up @@ -601,7 +605,8 @@ STARTOP(put_subs)
if (slot)
{
int index = seg.findClassIndex(input_class, slot->gid());
is->setGlyph(&seg, seg.getClassGlyph(output_class, index));
assert(index >=0 );
is->setGlyph(&seg, seg.getClassGlyph(output_class, static_cast<uint16>(index)));
Copy link

Choose a reason for hiding this comment

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

The question I would ask here for the sake of robustness: What does actually getClassGlyph return when called with an index like 31767 (from -1), and how does setGlyph react to getting fed that return value?

Copy link
Author

Choose a reason for hiding this comment

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

My initial intention of the code was limited to reducing warnings on locally building LibreOffice,which relies on this component, and haven't researched what would happen in that case, but making use of the opportunity of being asked, I tried to understand terms used in the code, like font,face,segment, slif , GDL and more. However, the links to useful information on README of this project were broken. Sorry, I'm currently at a loss.

For now, I found that I typo-ed "assert" as "asset". I will fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

no assert!

Copy link
Contributor

Choose a reason for hiding this comment

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

If getClassGlyph is called with a an index that is > than the limit it can handle, it returns 0 (and since the index is uint, it is never < 0). This gives us an adequate garbage in, garbage out error handling. The Graphite engine is not in an a position to return errors, it has to do something with whatever is thrown at it, even if the results are rubbish.

Copy link

Choose a reason for hiding this comment

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

Is perhaps the value or index 0 itself also classified as such a special reference value, or an error condition? The last comment seems to suggest so. E.g. the index 0 could indeed have been utilized as a valid but special index to hold, say, the "substitute glyph" to use as fallback or during "silent recovery" whenever the libray is thrown some bad input.

}
ENDOP

Expand Down