Last Comment Bug 710967 - Possible bug in AffixMgr::parse_convtable()
: Possible bug in AffixMgr::parse_convtable()
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla11
Assigned To: Jared Wein [:jaws] (please needinfo? me)
: Jet Villegas (:jet)
Depends on: hunspell-1.3.3
Blocks: 710966
  Show dependency treegraph
Reported: 2011-12-14 22:38 PST by Justin Dolske [:Dolske]
Modified: 2014-06-16 06:42 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch for bug 710967 (1.27 KB, patch)
2011-12-15 01:24 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Patch for bug 710967 (2.20 KB, patch)
2011-12-15 13:53 PST, Jared Wein [:jaws] (please needinfo? me) review+
ryanvm: feedback+
Details | Diff | Splinter Review

Description User image Justin Dolske [:Dolske] 2011-12-14 22:38:49 PST

Example N1. Incomplete verification for table integrity

int  AffixMgr::parse_convtable(..., const char * keyword)
  if (strncmp(piece, keyword, sizeof(keyword)) != 0) {
                       "error: line %d: table is corrupt\n",
      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).
Comment 1 User image Jared Wein [:jaws] (please needinfo? me) 2011-12-15 01:00:51 PST
We can fix this locally, but I'm not sure how to go about getting these changes pushed upstream.
Comment 2 User image Jared Wein [:jaws] (please needinfo? me) 2011-12-15 01:24:33 PST
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.
Comment 3 User image :Gavin Sharp [email:] 2011-12-15 08:24:40 PST
Nemeth might be able to advise re: fixing this upstream.
Comment 4 User image Ryan VanderMeulen [:RyanVM] 2011-12-15 10:28:50 PST
Please update README.hunspell with the bug number for this fix as well.
Comment 5 User image Jared Wein [:jaws] (please needinfo? me) 2011-12-15 13:53:32 PST
Created attachment 582101 [details] [diff] [review]
Patch for bug 710967

Updated README.hunspell with the bug number.
Comment 6 User image Jared Wein [:jaws] (please needinfo? me) 2011-12-15 22:11:17 PST
Comment 7 User image Caolan McNamara 2011-12-16 01:16:24 PST
committed this to upstream hunspell now as well, thanks for this
Comment 8 User image Rob Campbell [:rc] (:robcee) 2011-12-16 11:17:36 PST

Note You need to log in before you can comment on or make changes to this bug.