[erlang-patches] [PATCH] ei: integer overflow in string/atom encoding

[Michael Santos]

On Fri, Jun 03, 2011 at 06:21:10PM +0200, Mikael Pettersson wrote:
> Michael Santos writes:
>  > ei_encode_atom() and ei_encode_string() use strlen() to get the length
>  > of the buffer. As strlen() returns an unsigned long long and both ei
> 
> size_t is unsigned but may or may not be long long
> 
>  > functions take a signed integer, the length fields may overflow.
> 
> Indeed.
> 
>  > 
>  > Note: the index may still overflow.
>  > 
>  > Check the results of strlen can be held in a signed integer.
> 
> The change feels like a kludge.  IMO ei_encode_{atom,string}_len() should be
> fixed to take size_t lengths rather than signed int lengths.  Is there any
> reason why that wouldn't work?

I agree with the kludge comment. There may still be a need for a check
though since the list length is held in 4 bytes in the external term
format.

It would be great to move from signed to unsigned ints but that would
involve an interface change. If that's ok, I'd be happy to make the
changes and update the docs (looks like a few changes in other places
as well, like odbcserver.c).

> /Mikael
> 
> p.s. Thank you for inlining the patch and providing a rationale, most git
> users on this list don't do that which makes peer review difficult.
> 
>  > ---
>  >  lib/erl_interface/src/encode/encode_atom.c   |    4 ++++
>  >  lib/erl_interface/src/encode/encode_string.c |    6 +++++-
>  >  2 files changed, 9 insertions(+), 1 deletions(-)
>  > 
>  > diff --git a/lib/erl_interface/src/encode/encode_atom.c b/lib/erl_interface/src/encode/encode_atom.c
>  > index 69f2d14..0efa204 100644
>  > --- a/lib/erl_interface/src/encode/encode_atom.c
>  > +++ b/lib/erl_interface/src/encode/encode_atom.c
>  > @@ -17,12 +17,16 @@
>  >   * %CopyrightEnd%
>  >   */
>  >  #include <string.h>
>  > +#include <limits.h>
>  >  #include "eidef.h"
>  >  #include "eiext.h"
>  >  #include "putget.h"
>  >  
>  >  int ei_encode_atom(char *buf, int *index, const char *p)
>  >  {
>  > +    size_t len = strlen(p);
>  > +
>  > +    if (len >= INT_MAX) return -1;
>  >      return ei_encode_atom_len(buf, index, p, strlen(p));
>  >  }
>  >  
>  > diff --git a/lib/erl_interface/src/encode/encode_string.c b/lib/erl_interface/src/encode/encode_string.c
>  > index 1d342cb..593bbf2 100644
>  > --- a/lib/erl_interface/src/encode/encode_string.c
>  > +++ b/lib/erl_interface/src/encode/encode_string.c
>  > @@ -17,6 +17,7 @@
>  >   * %CopyrightEnd%
>  >   */
>  >  #include <string.h>
>  > +#include <limits.h>
>  >  #include "eidef.h"
>  >  #include "eiext.h"
>  >  #include "putget.h"
>  > @@ -24,7 +25,10 @@
>  >  
>  >  int ei_encode_string(char *buf, int *index, const char *p)
>  >  {
>  > -    return ei_encode_string_len(buf, index, p, strlen(p));
>  > +    size_t len = strlen(p);
>  > +
>  > +    if (len >= INT_MAX) return -1;
>  > +    return ei_encode_string_len(buf, index, p, len);
>  >  }
>  >  
>  >  int ei_encode_string_len(char *buf, int *index, const char *p, int len)
>  > -- 
>  > 1.7.0.4
>  > 
>  > _______________________________________________
>  > erlang-patches mailing list
>  > erlang-patches@REDACTED
>  > http://erlang.org/mailman/listinfo/erlang-patches
>  >