710967 - Possible bug in AffixMgr::parse_convtable()

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Justin Dolske [:Dolske]]

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)]

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)]

Attachment: 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: [e-mail]]]

Nemeth might be able to advise re: fixing this upstream.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Please update README.hunspell with the bug number for this fix as well.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 710967

Updated README.hunspell with the bug number.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/c6346cddd3c0

Comment 7

[Caolan McNamara]

committed this to upstream hunspell now as well, thanks for this

Comment 8

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/c6346cddd3c0