620170 - ParsePrivateDictData handles enum type in a way which doesn't guard local_subrs_index

Status: NEW

(Core :: Graphics, enhancement)

Description

[timeless]

328 bool ParsePrivateDictData(

407         ots::CFFIndex *local_subrs_index = NULL;
408         if (type == DICT_DATA_FDARRAY) {
412           local_subrs_index = out_cff->local_subrs_per_font.back();
413         } else if (type == DICT_DATA_TOPLEVEL) {
417           local_subrs_index = new ots::CFFIndex;
419         }

if type can only be DICT_DATA_FDARRAY or DICT_DATA_TOPLEVEL then 413 should be an else instead of else if. If it can be anything else, then we crash beneath ParseIndex:

420         if (!ParseIndex(&table, local_subrs_index)) {
421           return OTS_FAILURE();
422         }

51 bool ParseIndex(ots::Buffer *table, ots::CFFIndex *index) {
here:
52   index->off_size = 0;

Comment 1

[timeless]

the enum type today only has two values. it's nicer to hint to the compiler by using else instead of else if

Comment 2

[timeless]

Attachment: rough

i presume this is a third party module, and i have no idea what the style for the module is.

it would be helpful to both to compilers and coverity if the code was changed, this is one way of changing it, you could also omit the comment, or if you have an assertion language you could omit the comment and insert an assertion into the else block.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 498584 [details] [diff] [review]
rough

r-minusing this patch, as I don't think the issue justifies us patching this locally in Mozilla; however, I have reported this upstream to the Chromium project for their consideration. (http://code.google.com/p/chromium/issues/detail?id=93655)

If/when OTS decides what to do about this, we'll pick up any change/fix as part of an OTS update.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.