While running the parfait source checker over the X.Org sources, it returned a warning of: Error: Read buffer overflow at xf86-video-intel-2.6.1/src/i830_debug.c:1380 in function 'i830_valid_command' [Symbolic analysis] In array dereference of _3d_cmds[pipeline_type] with index 'pipeline_type' Array size is 4 elements (of 16384 bytes each), index >= 4 The relevant code in git master (starting at line 1485 now) is: pipeline_type = (cmd >> 27) & 0x3; [...] if (pipeline_type <= 3) return count; if (!_3d_cmds[pipeline_type][opcode][subopcode].name) return -1; It seems like the first if may be reversed and should be >= 3, though > 3 should be impossible given the & 0x3 in the assignment. [ This bug was found by the Parfait bug checking tool. For more information see http://research.sun.com/projects/parfait ]
It seems in case pipeline_type is sane, just return the count number. We might simply remove "if (pipeline_type <= 3) return count;" for cleanup.
Thanks for the report, Alan. It's easy to see how this code would confuse parfait. It confused us as well. :-) It seems fairly clear to me that the intent was to determine -1 if the command table has no name for the command being referenced. And the early return, (which was made unconditional based on the masking as you mentioned was preventing that). My inclination is to just remove the early return as Zhenyu mentions, (and as the patch below implements). But perhaps the early-return was meant to be a buffer-overflow protection, (which does seem a bit silly coming two-lines after the masking), so it would be legitimate to switch the sense of the condition as well. Anyway, I've posted my patch and I'll close this report when the patch is aplied. -Carl commit 2c19210b9d0224e61016d8cc76e1af28632fc0c1 Author: Carl Worth <cworth@cworth.org> Date: Fri Jul 31 11:20:23 2009 -0700 debug: i830_valid_command: Return invalid for subopcodes with no name Previously the code would always return the count, before ever looking into the _3d_cmds table to see if there was actually a valid command. Thanks to Alan Coopersmith who reported that the code was confusing parfait: https://bugs.freedesktop.org/show_bug.cgi?id=21666 diff --git a/src/i830_debug.c b/src/i830_debug.c index 1126c26..4b704a1 100644 --- a/src/i830_debug.c +++ b/src/i830_debug.c @@ -2094,8 +2094,6 @@ i830_valid_command (uint32_t cmd) count = 1; else count = (cmd & 0xff) + 2; - if (pipeline_type <= 3) - return count; if (!_3d_cmds[pipeline_type][opcode][subopcode].name) return -1; break;
The code has since been deleted in its entirety.
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.