[spice-common,v2] codegen: Check validity of array members

Submitted by Frediano Ziglio on Aug. 14, 2019, 5:08 p.m.

Details

Message ID 20190814170825.11903-1-fziglio@redhat.com
State Accepted
Commit d4248885e83d20180b965070383584977e46d682
Headers show
Series "codegen: Check validity of array members" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio Aug. 14, 2019, 5:08 p.m.
Check that combination of fields for an array does not
lead to unsafe code.
check_valid method came from generate_c_declaration with
some more check and it's use in demarshaller to validate
the array if the structure is not generated.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 python_modules/demarshal.py |  2 ++
 python_modules/ptypes.py    | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

Changes since v1:
- add comments to explain the checks done

Patch hide | download patch | download mbox

diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
index acd4b6f..3736976 100644
--- a/python_modules/demarshal.py
+++ b/python_modules/demarshal.py
@@ -315,6 +315,8 @@  def write_validate_pointer_item(writer, container, item, scope, parent_scope, st
 def write_validate_array_item(writer, container, item, scope, parent_scope, start,
                               want_nw_size, want_mem_size, want_extra_size):
     array = item.type
+    if item.member:
+        array.check_valid(item.member)
     is_byte_size = False
     element_type = array.element_type
     if array.is_bytes_length():
diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
index 311ce3d..ebe001c 100644
--- a/python_modules/ptypes.py
+++ b/python_modules/ptypes.py
@@ -485,7 +485,36 @@  class ArrayType(Type):
     def c_type(self):
         return self.element_type.c_type()
 
+    def check_valid(self, member):
+        # These attribute corresponds to specific structure size
+        if member.has_attr("chunk") or member.has_attr("as_ptr"):
+            return
+        # These attribute indicate that the array is stored in the structure
+        # as a pointer of the array. If there's no way to retrieve the length
+        # of the array give error, as the user has no way to do bound checks
+        if member.has_attr("to_ptr") or member.has_attr("ptr_array"):
+            if not (self.is_identifier_length() or self.is_constant_length()):
+                raise Exception("Unsecure, no length of array")
+            return
+        # This attribute indicate that the array is store at the end
+        # of the structure, the user will compute the length from the
+        # entire message size
+        if member.has_end_attr():
+            return
+        # Avoid bug, the array has no length specified and no space
+        # would be allocated
+        if self.is_remaining_length():
+            raise Exception('C output array is not allocated')
+        # For constant length (like "foo[5]") the field is a sized array
+        # For identifier automatically a pointer to allocated data is store,
+        # in this case user can read the size using the other field specified
+        # by the identifier
+        if self.is_constant_length() or self.is_identifier_length():
+            return
+        raise NotImplementedError('unknown array %s' % str(self))
+
     def generate_c_declaration(self, writer, member):
+        self.check_valid(member)
         name = member.name
         if member.has_attr("chunk"):
             return writer.writeln('SpiceChunks *%s;' % name)
@@ -497,7 +526,7 @@  class ArrayType(Type):
             return writer.writeln('%s *%s;' % (self.c_type(), name))
         if member.has_attr("ptr_array"):
             return writer.writeln('%s *%s[0];' % (self.c_type(), name))
-        if member.has_end_attr() or self.is_remaining_length():
+        if member.has_end_attr():
             return writer.writeln('%s %s[0];' % (self.c_type(), name))
         if self.is_constant_length():
             return writer.writeln('%s %s[%s];' % (self.c_type(), name, self.size))

Comments

ping

> 
> Check that combination of fields for an array does not
> lead to unsafe code.
> check_valid method came from generate_c_declaration with
> some more check and it's use in demarshaller to validate
> the array if the structure is not generated.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  python_modules/demarshal.py |  2 ++
>  python_modules/ptypes.py    | 31 ++++++++++++++++++++++++++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> Changes since v1:
> - add comments to explain the checks done
> 
> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> index acd4b6f..3736976 100644
> --- a/python_modules/demarshal.py
> +++ b/python_modules/demarshal.py
> @@ -315,6 +315,8 @@ def write_validate_pointer_item(writer, container, item,
> scope, parent_scope, st
>  def write_validate_array_item(writer, container, item, scope, parent_scope,
>  start,
>                                want_nw_size, want_mem_size, want_extra_size):
>      array = item.type
> +    if item.member:
> +        array.check_valid(item.member)
>      is_byte_size = False
>      element_type = array.element_type
>      if array.is_bytes_length():
> diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> index 311ce3d..ebe001c 100644
> --- a/python_modules/ptypes.py
> +++ b/python_modules/ptypes.py
> @@ -485,7 +485,36 @@ class ArrayType(Type):
>      def c_type(self):
>          return self.element_type.c_type()
>  
> +    def check_valid(self, member):
> +        # These attribute corresponds to specific structure size
> +        if member.has_attr("chunk") or member.has_attr("as_ptr"):
> +            return
> +        # These attribute indicate that the array is stored in the structure
> +        # as a pointer of the array. If there's no way to retrieve the
> length
> +        # of the array give error, as the user has no way to do bound checks
> +        if member.has_attr("to_ptr") or member.has_attr("ptr_array"):
> +            if not (self.is_identifier_length() or
> self.is_constant_length()):
> +                raise Exception("Unsecure, no length of array")
> +            return
> +        # This attribute indicate that the array is store at the end
> +        # of the structure, the user will compute the length from the
> +        # entire message size
> +        if member.has_end_attr():
> +            return
> +        # Avoid bug, the array has no length specified and no space
> +        # would be allocated
> +        if self.is_remaining_length():
> +            raise Exception('C output array is not allocated')
> +        # For constant length (like "foo[5]") the field is a sized array
> +        # For identifier automatically a pointer to allocated data is store,
> +        # in this case user can read the size using the other field
> specified
> +        # by the identifier
> +        if self.is_constant_length() or self.is_identifier_length():
> +            return
> +        raise NotImplementedError('unknown array %s' % str(self))
> +
>      def generate_c_declaration(self, writer, member):
> +        self.check_valid(member)
>          name = member.name
>          if member.has_attr("chunk"):
>              return writer.writeln('SpiceChunks *%s;' % name)
> @@ -497,7 +526,7 @@ class ArrayType(Type):
>              return writer.writeln('%s *%s;' % (self.c_type(), name))
>          if member.has_attr("ptr_array"):
>              return writer.writeln('%s *%s[0];' % (self.c_type(), name))
> -        if member.has_end_attr() or self.is_remaining_length():
> +        if member.has_end_attr():
>              return writer.writeln('%s %s[0];' % (self.c_type(), name))
>          if self.is_constant_length():
>              return writer.writeln('%s %s[%s];' % (self.c_type(), name,
>              self.size))
> --
> 2.20.1
> 
>
ping

> 
> ping
> 
> > 
> > Check that combination of fields for an array does not
> > lead to unsafe code.
> > check_valid method came from generate_c_declaration with
> > some more check and it's use in demarshaller to validate
> > the array if the structure is not generated.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  python_modules/demarshal.py |  2 ++
> >  python_modules/ptypes.py    | 31 ++++++++++++++++++++++++++++++-
> >  2 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > Changes since v1:
> > - add comments to explain the checks done
> > 
> > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > index acd4b6f..3736976 100644
> > --- a/python_modules/demarshal.py
> > +++ b/python_modules/demarshal.py
> > @@ -315,6 +315,8 @@ def write_validate_pointer_item(writer, container,
> > item,
> > scope, parent_scope, st
> >  def write_validate_array_item(writer, container, item, scope,
> >  parent_scope,
> >  start,
> >                                want_nw_size, want_mem_size,
> >                                want_extra_size):
> >      array = item.type
> > +    if item.member:
> > +        array.check_valid(item.member)
> >      is_byte_size = False
> >      element_type = array.element_type
> >      if array.is_bytes_length():
> > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > index 311ce3d..ebe001c 100644
> > --- a/python_modules/ptypes.py
> > +++ b/python_modules/ptypes.py
> > @@ -485,7 +485,36 @@ class ArrayType(Type):
> >      def c_type(self):
> >          return self.element_type.c_type()
> >  
> > +    def check_valid(self, member):
> > +        # These attribute corresponds to specific structure size
> > +        if member.has_attr("chunk") or member.has_attr("as_ptr"):
> > +            return
> > +        # These attribute indicate that the array is stored in the
> > structure
> > +        # as a pointer of the array. If there's no way to retrieve the
> > length
> > +        # of the array give error, as the user has no way to do bound
> > checks
> > +        if member.has_attr("to_ptr") or member.has_attr("ptr_array"):
> > +            if not (self.is_identifier_length() or
> > self.is_constant_length()):
> > +                raise Exception("Unsecure, no length of array")
> > +            return
> > +        # This attribute indicate that the array is store at the end
> > +        # of the structure, the user will compute the length from the
> > +        # entire message size
> > +        if member.has_end_attr():
> > +            return
> > +        # Avoid bug, the array has no length specified and no space
> > +        # would be allocated
> > +        if self.is_remaining_length():
> > +            raise Exception('C output array is not allocated')
> > +        # For constant length (like "foo[5]") the field is a sized array
> > +        # For identifier automatically a pointer to allocated data is
> > store,
> > +        # in this case user can read the size using the other field
> > specified
> > +        # by the identifier
> > +        if self.is_constant_length() or self.is_identifier_length():
> > +            return
> > +        raise NotImplementedError('unknown array %s' % str(self))
> > +
> >      def generate_c_declaration(self, writer, member):
> > +        self.check_valid(member)
> >          name = member.name
> >          if member.has_attr("chunk"):
> >              return writer.writeln('SpiceChunks *%s;' % name)
> > @@ -497,7 +526,7 @@ class ArrayType(Type):
> >              return writer.writeln('%s *%s;' % (self.c_type(), name))
> >          if member.has_attr("ptr_array"):
> >              return writer.writeln('%s *%s[0];' % (self.c_type(), name))
> > -        if member.has_end_attr() or self.is_remaining_length():
> > +        if member.has_end_attr():
> >              return writer.writeln('%s %s[0];' % (self.c_type(), name))
> >          if self.is_constant_length():
> >              return writer.writeln('%s %s[%s];' % (self.c_type(), name,
> >              self.size))
ping

> 
> ping
> 
> > 
> > ping
> > 
> > > 
> > > Check that combination of fields for an array does not
> > > lead to unsafe code.
> > > check_valid method came from generate_c_declaration with
> > > some more check and it's use in demarshaller to validate
> > > the array if the structure is not generated.
> > > 
> > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > ---
> > >  python_modules/demarshal.py |  2 ++
> > >  python_modules/ptypes.py    | 31 ++++++++++++++++++++++++++++++-
> > >  2 files changed, 32 insertions(+), 1 deletion(-)
> > > 
> > > Changes since v1:
> > > - add comments to explain the checks done
> > > 
> > > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > > index acd4b6f..3736976 100644
> > > --- a/python_modules/demarshal.py
> > > +++ b/python_modules/demarshal.py
> > > @@ -315,6 +315,8 @@ def write_validate_pointer_item(writer, container,
> > > item,
> > > scope, parent_scope, st
> > >  def write_validate_array_item(writer, container, item, scope,
> > >  parent_scope,
> > >  start,
> > >                                want_nw_size, want_mem_size,
> > >                                want_extra_size):
> > >      array = item.type
> > > +    if item.member:
> > > +        array.check_valid(item.member)
> > >      is_byte_size = False
> > >      element_type = array.element_type
> > >      if array.is_bytes_length():
> > > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > > index 311ce3d..ebe001c 100644
> > > --- a/python_modules/ptypes.py
> > > +++ b/python_modules/ptypes.py
> > > @@ -485,7 +485,36 @@ class ArrayType(Type):
> > >      def c_type(self):
> > >          return self.element_type.c_type()
> > >  
> > > +    def check_valid(self, member):
> > > +        # These attribute corresponds to specific structure size
> > > +        if member.has_attr("chunk") or member.has_attr("as_ptr"):
> > > +            return
> > > +        # These attribute indicate that the array is stored in the
> > > structure
> > > +        # as a pointer of the array. If there's no way to retrieve the
> > > length
> > > +        # of the array give error, as the user has no way to do bound
> > > checks
> > > +        if member.has_attr("to_ptr") or member.has_attr("ptr_array"):
> > > +            if not (self.is_identifier_length() or
> > > self.is_constant_length()):
> > > +                raise Exception("Unsecure, no length of array")
> > > +            return
> > > +        # This attribute indicate that the array is store at the end
> > > +        # of the structure, the user will compute the length from the
> > > +        # entire message size
> > > +        if member.has_end_attr():
> > > +            return
> > > +        # Avoid bug, the array has no length specified and no space
> > > +        # would be allocated
> > > +        if self.is_remaining_length():
> > > +            raise Exception('C output array is not allocated')
> > > +        # For constant length (like "foo[5]") the field is a sized array
> > > +        # For identifier automatically a pointer to allocated data is
> > > store,
> > > +        # in this case user can read the size using the other field
> > > specified
> > > +        # by the identifier
> > > +        if self.is_constant_length() or self.is_identifier_length():
> > > +            return
> > > +        raise NotImplementedError('unknown array %s' % str(self))
> > > +
> > >      def generate_c_declaration(self, writer, member):
> > > +        self.check_valid(member)
> > >          name = member.name
> > >          if member.has_attr("chunk"):
> > >              return writer.writeln('SpiceChunks *%s;' % name)
> > > @@ -497,7 +526,7 @@ class ArrayType(Type):
> > >              return writer.writeln('%s *%s;' % (self.c_type(), name))
> > >          if member.has_attr("ptr_array"):
> > >              return writer.writeln('%s *%s[0];' % (self.c_type(), name))
> > > -        if member.has_end_attr() or self.is_remaining_length():
> > > +        if member.has_end_attr():
> > >              return writer.writeln('%s %s[0];' % (self.c_type(), name))
> > >          if self.is_constant_length():
> > >              return writer.writeln('%s %s[%s];' % (self.c_type(), name,
> > >              self.size))
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
ping

> 
> ping
> 
> > 
> > ping
> > 
> > > 
> > > ping
> > > 
> > > > 
> > > > Check that combination of fields for an array does not
> > > > lead to unsafe code.
> > > > check_valid method came from generate_c_declaration with
> > > > some more check and it's use in demarshaller to validate
> > > > the array if the structure is not generated.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > > ---
> > > >  python_modules/demarshal.py |  2 ++
> > > >  python_modules/ptypes.py    | 31 ++++++++++++++++++++++++++++++-
> > > >  2 files changed, 32 insertions(+), 1 deletion(-)
> > > > 
> > > > Changes since v1:
> > > > - add comments to explain the checks done
> > > > 
> > > > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > > > index acd4b6f..3736976 100644
> > > > --- a/python_modules/demarshal.py
> > > > +++ b/python_modules/demarshal.py
> > > > @@ -315,6 +315,8 @@ def write_validate_pointer_item(writer, container,
> > > > item,
> > > > scope, parent_scope, st
> > > >  def write_validate_array_item(writer, container, item, scope,
> > > >  parent_scope,
> > > >  start,
> > > >                                want_nw_size, want_mem_size,
> > > >                                want_extra_size):
> > > >      array = item.type
> > > > +    if item.member:
> > > > +        array.check_valid(item.member)
> > > >      is_byte_size = False
> > > >      element_type = array.element_type
> > > >      if array.is_bytes_length():
> > > > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > > > index 311ce3d..ebe001c 100644
> > > > --- a/python_modules/ptypes.py
> > > > +++ b/python_modules/ptypes.py
> > > > @@ -485,7 +485,36 @@ class ArrayType(Type):
> > > >      def c_type(self):
> > > >          return self.element_type.c_type()
> > > >  
> > > > +    def check_valid(self, member):
> > > > +        # These attribute corresponds to specific structure size
> > > > +        if member.has_attr("chunk") or member.has_attr("as_ptr"):
> > > > +            return
> > > > +        # These attribute indicate that the array is stored in the
> > > > structure
> > > > +        # as a pointer of the array. If there's no way to retrieve the
> > > > length
> > > > +        # of the array give error, as the user has no way to do bound
> > > > checks
> > > > +        if member.has_attr("to_ptr") or member.has_attr("ptr_array"):
> > > > +            if not (self.is_identifier_length() or
> > > > self.is_constant_length()):
> > > > +                raise Exception("Unsecure, no length of array")
> > > > +            return
> > > > +        # This attribute indicate that the array is store at the end
> > > > +        # of the structure, the user will compute the length from the
> > > > +        # entire message size
> > > > +        if member.has_end_attr():
> > > > +            return
> > > > +        # Avoid bug, the array has no length specified and no space
> > > > +        # would be allocated
> > > > +        if self.is_remaining_length():
> > > > +            raise Exception('C output array is not allocated')
> > > > +        # For constant length (like "foo[5]") the field is a sized
> > > > array
> > > > +        # For identifier automatically a pointer to allocated data is
> > > > store,
> > > > +        # in this case user can read the size using the other field
> > > > specified
> > > > +        # by the identifier
> > > > +        if self.is_constant_length() or self.is_identifier_length():
> > > > +            return
> > > > +        raise NotImplementedError('unknown array %s' % str(self))
> > > > +
> > > >      def generate_c_declaration(self, writer, member):
> > > > +        self.check_valid(member)
> > > >          name = member.name
> > > >          if member.has_attr("chunk"):
> > > >              return writer.writeln('SpiceChunks *%s;' % name)
> > > > @@ -497,7 +526,7 @@ class ArrayType(Type):
> > > >              return writer.writeln('%s *%s;' % (self.c_type(), name))
> > > >          if member.has_attr("ptr_array"):
> > > >              return writer.writeln('%s *%s[0];' % (self.c_type(),
> > > >              name))
> > > > -        if member.has_end_attr() or self.is_remaining_length():
> > > > +        if member.has_end_attr():
> > > >              return writer.writeln('%s %s[0];' % (self.c_type(), name))
> > > >          if self.is_constant_length():
> > > >              return writer.writeln('%s %s[%s];' % (self.c_type(), name,
> > > >              self.size))
ping

> 
> ping
> 
> > 
> > ping
> > 
> > > 
> > > ping
> > > 
> > > > 
> > > > ping
> > > > 
> > > > > 
> > > > > Check that combination of fields for an array does not
> > > > > lead to unsafe code.
> > > > > check_valid method came from generate_c_declaration with
> > > > > some more check and it's use in demarshaller to validate
> > > > > the array if the structure is not generated.
> > > > > 
> > > > > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > > > > ---
> > > > >  python_modules/demarshal.py |  2 ++
> > > > >  python_modules/ptypes.py    | 31 ++++++++++++++++++++++++++++++-
> > > > >  2 files changed, 32 insertions(+), 1 deletion(-)
> > > > > 
> > > > > Changes since v1:
> > > > > - add comments to explain the checks done
> > > > > 
> > > > > diff --git a/python_modules/demarshal.py
> > > > > b/python_modules/demarshal.py
> > > > > index acd4b6f..3736976 100644
> > > > > --- a/python_modules/demarshal.py
> > > > > +++ b/python_modules/demarshal.py
> > > > > @@ -315,6 +315,8 @@ def write_validate_pointer_item(writer,
> > > > > container,
> > > > > item,
> > > > > scope, parent_scope, st
> > > > >  def write_validate_array_item(writer, container, item, scope,
> > > > >  parent_scope,
> > > > >  start,
> > > > >                                want_nw_size, want_mem_size,
> > > > >                                want_extra_size):
> > > > >      array = item.type
> > > > > +    if item.member:
> > > > > +        array.check_valid(item.member)
> > > > >      is_byte_size = False
> > > > >      element_type = array.element_type
> > > > >      if array.is_bytes_length():
> > > > > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > > > > index 311ce3d..ebe001c 100644
> > > > > --- a/python_modules/ptypes.py
> > > > > +++ b/python_modules/ptypes.py
> > > > > @@ -485,7 +485,36 @@ class ArrayType(Type):
> > > > >      def c_type(self):
> > > > >          return self.element_type.c_type()
> > > > >  
> > > > > +    def check_valid(self, member):
> > > > > +        # These attribute corresponds to specific structure size
> > > > > +        if member.has_attr("chunk") or member.has_attr("as_ptr"):
> > > > > +            return
> > > > > +        # These attribute indicate that the array is stored in the
> > > > > structure
> > > > > +        # as a pointer of the array. If there's no way to retrieve
> > > > > the
> > > > > length
> > > > > +        # of the array give error, as the user has no way to do
> > > > > bound
> > > > > checks
> > > > > +        if member.has_attr("to_ptr") or
> > > > > member.has_attr("ptr_array"):
> > > > > +            if not (self.is_identifier_length() or
> > > > > self.is_constant_length()):
> > > > > +                raise Exception("Unsecure, no length of array")
> > > > > +            return
> > > > > +        # This attribute indicate that the array is store at the end
> > > > > +        # of the structure, the user will compute the length from
> > > > > the
> > > > > +        # entire message size
> > > > > +        if member.has_end_attr():
> > > > > +            return
> > > > > +        # Avoid bug, the array has no length specified and no space
> > > > > +        # would be allocated
> > > > > +        if self.is_remaining_length():
> > > > > +            raise Exception('C output array is not allocated')
> > > > > +        # For constant length (like "foo[5]") the field is a sized
> > > > > array
> > > > > +        # For identifier automatically a pointer to allocated data
> > > > > is
> > > > > store,
> > > > > +        # in this case user can read the size using the other field
> > > > > specified
> > > > > +        # by the identifier
> > > > > +        if self.is_constant_length() or self.is_identifier_length():
> > > > > +            return
> > > > > +        raise NotImplementedError('unknown array %s' % str(self))
> > > > > +
> > > > >      def generate_c_declaration(self, writer, member):
> > > > > +        self.check_valid(member)
> > > > >          name = member.name
> > > > >          if member.has_attr("chunk"):
> > > > >              return writer.writeln('SpiceChunks *%s;' % name)
> > > > > @@ -497,7 +526,7 @@ class ArrayType(Type):
> > > > >              return writer.writeln('%s *%s;' % (self.c_type(), name))
> > > > >          if member.has_attr("ptr_array"):
> > > > >              return writer.writeln('%s *%s[0];' % (self.c_type(),
> > > > >              name))
> > > > > -        if member.has_end_attr() or self.is_remaining_length():
> > > > > +        if member.has_end_attr():
> > > > >              return writer.writeln('%s %s[0];' % (self.c_type(),
> > > > >              name))
> > > > >          if self.is_constant_length():
> > > > >              return writer.writeln('%s %s[%s];' % (self.c_type(),
> > > > >              name,
> > > > >              self.size))