Fixes for multiple potential out-of-bounds reads in hunspell::NodeReader....

Fixes for multiple potential out-of-bounds reads in hunspell::NodeReader.

BUG=14721
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30191

[jschuh, 2009-10-23 18:09:38]

Adds several buffer position checks in NodeReader methods, and fixes two checks
in BDictReader::Init(). This should address the crashes we're still seeing from
bug 14721.

[brettw, 2009-10-23 19:47:07]

http://codereview.chromium.org/326013/diff/4001/3002
File third_party/hunspell/google/bdict_reader.cc (right):

http://codereview.chromium.org/326013/diff/4001/3002#newcode185
Line 185: return 0;  // don't continue with a corrupt node
Check that your new comments begin with a capital and end with a period.

http://codereview.chromium.org/326013/diff/4001/3002#newcode313
Line 313: if (!is_valid_)
It seems like this should be at the top of the function. Why did you put it
here?

http://codereview.chromium.org/326013/diff/4001/3002#newcode356
Line 356: !is_valid_)
Do you also want to do this at the top? It seems like we wouldn't want to check
lookup_has_0th() if the node is invalid.

[jschuh, 2009-10-23 21:09:25]

Found a missed length check, removed some redundant checks, and added a comment.

http://codereview.chromium.org/326013/diff/4001/3002
File third_party/hunspell/google/bdict_reader.cc (right):

http://codereview.chromium.org/326013/diff/4001/3002#newcode185
Line 185: return 0;  // don't continue with a corrupt node
On 2009/10/23 19:47:07, brettw wrote:
> Check that your new comments begin with a capital and end with a period.

Done.

http://codereview.chromium.org/326013/diff/4001/3002#newcode313
Line 313: if (!is_valid_)
The is_valid_ member may get set inside leaf_has_following(). However, it should
also be checked inside affix_id_for_leaf(), which I added in the most recent
patch set. 

I can remove this check, as we won't actually touch the contents of the buffer
until the loop, in which case we'll check the buffer position first and hit the
error return condition.

On 2009/10/23 19:47:07, brettw wrote:
> It seems like this should be at the top of the function. Why did you put it
> here?

http://codereview.chromium.org/326013/diff/4001/3002#newcode356
Line 356: !is_valid_)
None of the code up to that point in the function can touch the buffer without
checking is_valid_ implicitly and setting it to false if we're outside the
bounds. However, I can also remove this check because if we fall through it will
be performed in ReaderForLookupAt(). 

On 2009/10/23 19:47:07, brettw wrote:
> Do you also want to do this at the top? It seems like we wouldn't want to
check
> lookup_has_0th() if the node is invalid.

[brettw, 2009-10-23 21:23:23]

LGTM

[jschuh, 2012-07-20 23:16:16]

lgtm with a nit.

http://codereview.chromium.org/326013/diff/5003/third_party/hunspell/google/b...
File third_party/hunspell/google/bdict_reader.cc (right):

http://codereview.chromium.org/326013/diff/5003/third_party/hunspell/google/b...
third_party/hunspell/google/bdict_reader.cc:51: BDict::LEAF_NODE_TYPE_VALUE &&
is_valid_;
Since id_byte() does an is_valid_ check this is redundant. I'd remove it.