Possible bug in AffixMgr::parse_convtable()

RESOLVED FIXED in mozilla11

Status

()

Core
Spelling checker
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Dolske, Assigned: jaws)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [pvs-studio][fixed-in-fx-team][fixed-in-hunspell-1.3.3])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
From http://www.viva64.com/en/a/0078/

Example N1. Incomplete verification for table integrity

int  AffixMgr::parse_convtable(..., const char * keyword)
{
  ...
  if (strncmp(piece, keyword, sizeof(keyword)) != 0) {
      HUNSPELL_WARNING(stderr,
                       "error: line %d: table is corrupt\n",
                       af->getlinenum());
      delete *rl;
      *rl = NULL;
      return 1;
  }
  ...
}

PVS-Studio diagnostic message: V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. affixmgr.cpp 3708

The programmer tried to verify the table integrity here. Unfortunately, this check may both work and fail. To calculate the length of the key word the sizeof() operator is used, which is certainly incorrect. As a result, whether or not the code works will depend on pure luck (at certain values of the key word and 'keyword' pointer's size in the current data model).
(Reporter)

Updated

5 years ago
Blocks: 710966
We can fix this locally, but I'm not sure how to go about getting these changes pushed upstream.
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Created attachment 581914 [details] [diff] [review]
Patch for bug 710967

I couldn't find a list of potential reviewers. Please redirect review request as necessary.

As far as the patch goes, the only callers of parse_convtable are internal to this file and only call this function with one of two constant arguments for |keyword| ("OCONV" or "ICONV"). As such, I'm not concerned about a buffer overrun here with the use of |strlen| since it is not using user-generated input.
Attachment #581914 - Flags: review?(gavin.sharp)
OS: Mac OS X → All
Hardware: x86 → All
Nemeth might be able to advise re: fixing this upstream.
Please update README.hunspell with the bug number for this fix as well.
Created attachment 582101 [details] [diff] [review]
Patch for bug 710967

Updated README.hunspell with the bug number.
Attachment #581914 - Attachment is obsolete: true
Attachment #581914 - Flags: review?(gavin.sharp)
Attachment #582101 - Flags: review?(gavin.sharp)
Attachment #582101 - Flags: feedback?(ryanvm)
Attachment #582101 - Flags: review?(gavin.sharp) → review+
Attachment #582101 - Flags: feedback?(ryanvm) → feedback+
https://hg.mozilla.org/integration/fx-team/rev/c6346cddd3c0
Whiteboard: [pvs-studio] → [pvs-studio][fixed-in-fx-team]

Comment 7

5 years ago
committed this to upstream hunspell now as well, thanks for this
https://hg.mozilla.org/mozilla-central/rev/c6346cddd3c0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Whiteboard: [pvs-studio][fixed-in-fx-team] → [pvs-studio][fixed-in-fx-team][fixed-in-hunspell-1.3.3]
Depends on: 1022262
You need to log in before you can comment on or make changes to this bug.