Last Comment Bug 710967 - Possible bug in AffixMgr::parse_convtable()
: Possible bug in AffixMgr::parse_convtable()
Status: RESOLVED FIXED
[pvs-studio][fixed-in-fx-team][fixed-...
:
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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)
gavin.sharp: review+
ryanvm: feedback+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2011-12-14 22:38:49 PST
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).
Comment 1 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 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 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-15 08:24:40 PST
Nemeth might be able to advise re: fixing this upstream.
Comment 4 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 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 Jared Wein [:jaws] (please needinfo? me) 2011-12-15 22:11:17 PST
https://hg.mozilla.org/integration/fx-team/rev/c6346cddd3c0
Comment 7 Caolan McNamara 2011-12-16 01:16:24 PST
committed this to upstream hunspell now as well, thanks for this
Comment 8 Rob Campbell [:rc] (:robcee) 2011-12-16 11:17:36 PST
https://hg.mozilla.org/mozilla-central/rev/c6346cddd3c0

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