Skip to content

Commit

Permalink
patch 8.0.0322: possible overflow with corrupted spell file
Browse files Browse the repository at this point in the history 
Problem:    Possible overflow with spell file where the tree length is
            corrupted.
Solution:   Check for an invalid length (suggested by shqking)
  • Loading branch information
@brammool
brammool committed Feb 9, 2017
1 parent 8cc2a9c commit 399c297
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 0 deletions.
3 changes: 3 additions & 0 deletions src/spellfile.c
View file
Expand Up @@ -1595,6 +1595,9 @@ spell_read_tree(
len = get4c(fd);
if (len < 0)
return SP_TRUNCERROR;
if (len >= 0x3ffffff)
/* Invalid length, multiply with sizeof(int) would overflow. */
return SP_FORMERROR;
if (len > 0)
{
/* Allocate the byte array. */
Expand Down
2 changes: 2 additions & 0 deletions src/version.c
View file
Expand Up @@ -764,6 +764,8 @@ static char *(features[]) =

static int included_patches[] =
{ /* Add new patch number below this line */
/**/
322,
/**/
321,
/**/
Expand Down

8 comments on commit 399c297

@xtspackages
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial patch submitted by shqking (https://groups.google.com/forum/#!topic/vim_dev/t-3RSdEnrHY) used an upper bound of 0x3fffffff (note there are 7 'f's) while the upper bound that was commited is 0x3ffffff (6 'f's). Is this correct? Also, is there a #DEFINE or global that can be used instead of this seemingly arbitrary hardcoded number?

@shqking
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi asdofijanw,
Thanks for your comment.

I guess it is brammool's written error.
The upper bound should be 0x3fffffff( i.e. 7 'f's).

@jamessan
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is there a #DEFINE or global that can be used instead of this seemingly arbitrary hardcoded number?

Yeah, it'd be better to do something based on the actual bounds of the data types, like if (len >= (ULONG_MAX / sizeof(int)).

@shqking
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it'd be better to do something based on the actual bounds of the data types, like if (len >= (ULONG_MAX / sizeof(int)).

Yes. I agree with you.

@MaskRay
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it'd be better to do something based on the actual bounds of the data types, like if (len >= (ULONG_MAX / sizeof(int)).

len > ULONG_MAX / sizeof(int) ?

@shqking
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len > ULONG_MAX / sizeof(int) ?

#define UINT_MAX 0xffffffff // 2^32 - 1, the max value that UNSIGNED INT can represent.
len > UINT_MAX / sizeof(int)

@k-takata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @shqking. The type of len is int, so UINT_MAX should be used.

@brammool Why not include this?

--- a/src/spellfile.c
+++ b/src/spellfile.c
@@ -1595,7 +1595,7 @@ spell_read_tree(
     len = get4c(fd);
     if (len < 0)
 	return SP_TRUNCERROR;
-    if (len >= 0x3ffffff)
+    if (len > UINT_MAX / sizeof(int))
 	/* Invalid length, multiply with sizeof(int) would overflow. */
 	return SP_FORMERROR;
     if (len > 0)

@brammool
Copy link
Contributor Author

@brammool brammool commented on 399c297 Feb 26, 2017 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.