Bug 18064 - Making message validation more efficient for fixed-size type arrays
Summary: Making message validation more efficient for fixed-size type arrays
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: medium enhancement
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard:
Keywords:
: 14402 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-10-14 11:24 UTC by Jon Gosting
Modified: 2008-11-10 20:33 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Patch file for dbus-marshal-validate.c (2.29 KB, patch)
2008-10-14 11:24 UTC, Jon Gosting
Details | Splinter Review
Revised patch for dbus-marshal-validate.c (3.66 KB, patch)
2008-10-16 09:50 UTC, Jon Gosting
Details | Splinter Review

Description Jon Gosting 2008-10-14 11:24:51 UTC
Created attachment 19653 [details] [review]
Patch file for dbus-marshal-validate.c

I modified the function validate_body_helper in dbus-marshal-validate.c.  Previously, when it validated an array it would call itself once for each element.  This causes a significant performance hit for large arrays.  Also, for fixed-size types other than booleans, there is no real validation for it perform.  Before validate_body_helper calls itself for each element, it has already checked the size of the array and the alignment of the first element.

The modified version of validate_body_helper checks to see if the element type of the array is a fixed-size type.  If the element type is DBUS_TYPE_BOOLEAN, each element's value is checked to make sure it is valid.  If the element type is any of the other fixed-size types, the rest of the array is skipped since no further validation is needed.  

All other types are treated in the same way as before, with one recursive call for each element.

This issue was discussed in dbus Digest, Vol 39, Issue 8.  I worked with the sources from the 1.2.4 release.

Thanks!
Comment 1 Havoc Pennington 2008-10-14 11:36:30 UTC
Thanks, some minor comments

+                int array_elem_type

This is declared in the middle of a block it looks like, has to be at the top.

+                if (dbus_type_is_fixed (array_elem_type))

I think type_is_fixed assumes the type is a valid type, I didn't look but should be sure the code has already validated the type.

+                //

Should use C-style comments

+                        dbus_uint32_t v = 0;

Again have to declare at start of the block (also dbus doesn't usually mix declaration and initialization with a value)

Comment 2 Jon Gosting 2008-10-16 09:50:09 UTC
Created attachment 19693 [details] [review]
Revised patch for dbus-marshal-validate.c

I made all of the requested changes to the patch regarding C++ style comments, variable declarations, etc.  Also, dbus_type_is_fixed does not handle invalid types so I check that the element type is valid before using it.  

In the original file on line 377 the alignment of the pointer was adjusted for the array element type if necessary, however, the element type wasn't being checked for validity beforehand.  Passing an invalid element type in caused the pointer to be set to NULL.  Also, when the pointer did need to be adjusted, there was no check that the proper padding value was present in the array.  I added a check of the array element type and a check for the padding when necessary to this section of code.   

I used this patch with the 1.2.4 version of libdbus.  It passed all of the tests run by dbus-test.

Thanks!
Comment 3 Jon Gosting 2008-10-16 09:53:37 UTC
I made all of the requested changes to the patch regarding C++ style comments, variable declarations, etc.  Also, dbus_type_is_fixed does not handle invalid types so I check that the element type is valid before using it.  

In the original file on line 377 the alignment of the pointer was adjusted for the array element type if necessary, however, the element type wasn't being checked for validity beforehand.  Passing an invalid element type in caused the pointer to be set to NULL.  Also, when the pointer did need to be adjusted, there was no check that the proper padding value was present in the array.  I added a check of the array element type and a check for the padding when necessary to this section of code.   

I used this patch with the 1.2.4 version of libdbus.  It passed all of the tests run by dbus-test.

Thanks!
Comment 4 John (J5) Palmieri 2008-10-16 11:23:00 UTC
isn't this a dup of 14402?  http://bugs.freedesktop.org/show_bug.cgi?id=14402 

I thought this was applied to head awhile back but I guess it slipped through the cracks (we discussed it on the mailing list I think).
Comment 5 Jon Gosting 2008-10-16 11:58:23 UTC
Yes.  Sorry, I missed it in Bugzilla.  I noticed it just before I submitted
my second patch.  However, the patch submitted in 14402 doesn't verify that
the element type is valid or that the value of bools are legal, so I
submitted my patch anyway.  Sorry, I should have mentioned this in the
comment section of the bug report.

On Thu, Oct 16, 2008 at 11:23 AM, <bugzilla-daemon@freedesktop.org> wrote:

> http://bugs.freedesktop.org/show_bug.cgi?id=18064
>
>
>
>
>
> --- Comment #4 from John (J5) Palmieri <johnp@redhat.com>  2008-10-16
> 11:23:00 PST ---
> isn't this a dup of 14402?
> http://bugs.freedesktop.org/show_bug.cgi?id=14402
>
> I thought this was applied to head awhile back but I guess it slipped
> through
> the cracks (we discussed it on the mailing list I think).
>
>
> --
> Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
>
Comment 6 Colin Walters 2008-11-10 20:31:46 UTC
*** Bug 14402 has been marked as a duplicate of this bug. ***
Comment 7 Colin Walters 2008-11-10 20:33:19 UTC
commit e61f13cf328d131ddbd8b49842fcd0f49847dbff
Author: Jon Gosting <yukarionsen@gmail.com>
Date:   Mon Nov 10 23:29:05 2008 -0500

    Bug 18064 - more efficient validation for fixed-size type arrays
    
    	* dbus/dbus-marshal-validate.c: If an array is fixed size,
    	skip validation
    
    Signed-off-by: Colin Walters <walters@verbum.org>



Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.