On 15/08/18 10:45, andrey simiklit wrote:
Hi all,
Thanks for your reply.
We shouldn't even get to use the iterator if it's an unknown
instruction.
The decoder should just advance dword by dword until it finds
something that
makes sense again.
Got it)
So this is an expected behavior there:
return iter_group_offset_bits(iter, iter->group_iter + 1) <
(gen_group_get_length(iter->group, iter->p) * 32);
when we convert a negative *int* to *uint* to return true to continue
our loop.
return iter_group_offset_bits(iter, iter->group_iter + 1) <
(*0xFFFFFFE0U*);
Do you think it is good idea to add comment or something like this
into the "iter_more_groups" function:
int *length* = gen_group_get_length(iter->group, iter->p);
return *length < 0 ||*
iter_group_offset_bits(iter, iter->group_iter + 1) <
(*length* * 32);
to show more explicitly here that we want to return true to continue
our loop
when the -1 is returned from the "gen_group_get_length" function
because at the moment it is a bit implicit)
Please let me know if I am incorrect.
Sorry for the late answer :(
This implies an unknown size for the inspected instruction/struct.
I think this shouldn't happen because the caller even try to initialize
the iterator to decode it.
I would add an assert, because the iterator doesn't really deal with
that case.
I'm not sure whether there is such case in the genxml files, but if it
happens we should probably look into it.
Cheers,
-
Lionel
Regards,
Andrii.
On Tue, Aug 14, 2018 at 7:08 PM, Lionel Landwerlin
<[email protected] <mailto:[email protected]>>
wrote:
On 14/08/18 16:16, Rafael Antognolli wrote:
On Tue, Aug 14, 2018 at 03:36:18PM +0100, Lionel Landwerlin wrote:
On 14/08/18 12:55, asimiklit.work <http://asimiklit.work>
wrote:
Hi Lionel,
Hi Andrii,
Again sorry, I don't think this is the right fix.
I'm sending another patch to fix the parsing of
MI_BATCH_BUFFER_START which seems to be the actual
issue.
Thanks for working on this,
Thanks for your fast reply.
I agree that it is not correct patch for this issue
but anyway
"iter_more_groups" function will still work incorrectly
for unknown instructions when the
"iter->group->variable" field is true.
I guess that this case should be fixed.
Please let me know if I am incorrect.
Hey Andrii,
We shouldn't even get to use the iterator if it's an
unknown instruction.
The decoder should just advance dword by dword until it
finds something that
makes sense again.
If we run into that problem, I think we should fix the caller.
In that case, would an unreachable() or assert be a good thing
to do?
Yep, I guess assert in gen_field_iterator_init() should be a good
thing.
Regards,
Andrii.
On 2018-08-14 1:26 PM, Lionel Landwerlin wrote:
Hi Andrii,
Again sorry, I don't think this is the right fix.
I'm sending another patch to fix the parsing of
MI_BATCH_BUFFER_START which seems to be the actual
issue.
Thanks for working on this,
-
Lionel
On 14/08/18 10:04, [email protected]
<mailto:[email protected]> wrote:
From: Andrii Simiklit
<[email protected]
<mailto:[email protected]>>
The "gen_group_get_length" function can return
a negative value
and it can lead to the out of bounds group_iter.
v2: printing of "unknown command type" was added
Bugzilla:
https://bugs.freedesktop.org/show_bug.cgi?id=107544
<https://bugs.freedesktop.org/show_bug.cgi?id=107544>
Signed-off-by: Andrii Simiklit
<[email protected]
<mailto:[email protected]>>
---
src/intel/common/gen_decoder.c | 13
+++++++++++--
1 file changed, 11 insertions(+), 2
deletions(-)
diff --git a/src/intel/common/gen_decoder.c
b/src/intel/common/gen_decoder.c
index ec0a486..b36facf 100644
--- a/src/intel/common/gen_decoder.c
+++ b/src/intel/common/gen_decoder.c
@@ -770,6 +770,13 @@
gen_group_get_length(struct gen_group
*group, const uint32_t *p)
return -1;
}
}
+ default: {
+ fprintf(stderr, "Unknown command type
%u in '%s::%s'\n",
+ type,
+ (group->parent &&
group->parent->name) ?
group->parent->name : "UNKNOWN",
+ group->name ? group->name :
"UNKNOWN");
+ break;
+ }
}
return -1;
@@ -803,8 +810,10 @@ static bool
iter_more_groups(const struct
gen_field_iterator *iter)
{
if (iter->group->variable) {
- return iter_group_offset_bits(iter,
iter->group_iter + 1) <
-
(gen_group_get_length(iter->group, iter->p) * 32);
+ const int length =
gen_group_get_length(iter->group, iter->p);
+ return length > 0 &&
+ iter_group_offset_bits(iter,
iter->group_iter + 1) <
+ (length * 32);
} else {
return (iter->group_iter + 1) <
iter->group->group_count ||
iter->group->next != NULL;
_______________________________________________
mesa-dev mailing list
[email protected]
<mailto:[email protected]>
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
<https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
