| Message ID | 20171117162055.7128-1-derek.buitenhuis@gmail.com |
|---|---|
| State | New |
| Headers | show |
2017-11-17 17:20 GMT+01:00 Derek Buitenhuis <derek.buitenhuis@gmail.com>: > mb_area_start has 5 entries, and 'a' is iterated through from 0 to 3. > 'a2' is set to 'a + 1', and mb_area_start[a2 + 1] is accessed, so if > a is 3, then we try to access mb_area_start[5]. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > I'm not 100% sure if this fix is /correct/, so hopefully someone > knows the DV code... > --- > libavcodec/dvenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/dvenc.c b/libavcodec/dvenc.c > index ce2fc75daa..b79cbebb04 100644 > --- a/libavcodec/dvenc.c > +++ b/libavcodec/dvenc.c > @@ -383,7 +383,7 @@ static inline void dv_guess_qnos(EncBlockInfo *blks, > int *qnos) > prev = k; > } else { > if (b->next[k] >= mb_area_start[a + 1] && > b->next[k] < 64) { > - for (a2 = a + 1; b->next[k] >= > mb_area_start[a2 + 1]; a2++) > + for (a2 = a + 1; a2 < 4 && b->next[k] > >= mb_area_start[a2 + 1]; a2++) > b->prev[a2] = prev; > av_assert2(a2 < 4); > av_assert2(b->mb[b->next[k]]); > -- > > Hello, doesn't know the dvenc code, but you seems to test the assert of the next line Maybe move the assert (a2 < 4); before the for loop, if it's a theorical case, or remove it if this case can really happen. Martin
On 11/17/2017 4:42 PM, Martin Vignali wrote: > doesn't know the dvenc code, > but you seems to test the assert of the next line > > Maybe move the assert (a2 < 4); before the for loop, if it's a theorical > case, > or remove it if this case can really happen. I don't see anything that would prevent it from happening, but the code is also about as clear as mud. In general, I think I prefer a real check to an assert, unless someone can explain why it *wouldn't* happen. After git blame-ing my way through roughly 166393765431 "K&R formatting" and "refactor" commits, the original code comes from the ancient commit of d2d230a7569154306a1625ca37dbfa4c36627ec6 which provides no info whatsoever on why it is correct at all. CCing Michael, as he authored that commit - maybe he can provide insight. - Derek
On Fri, Nov 17, 2017 at 04:20:55PM +0000, Derek Buitenhuis wrote: > mb_area_start has 5 entries, and 'a' is iterated through from 0 to 3. > 'a2' is set to 'a + 1', and mb_area_start[a2 + 1] is accessed, so if > a is 3, then we try to access mb_area_start[5]. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > I'm not 100% sure if this fix is /correct/, so hopefully someone > knows the DV code... > --- > libavcodec/dvenc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/dvenc.c b/libavcodec/dvenc.c > index ce2fc75daa..b79cbebb04 100644 > --- a/libavcodec/dvenc.c > +++ b/libavcodec/dvenc.c > @@ -383,7 +383,7 @@ static inline void dv_guess_qnos(EncBlockInfo *blks, int *qnos) > prev = k; > } else { > if (b->next[k] >= mb_area_start[a + 1] && b->next[k] < 64) { > - for (a2 = a + 1; b->next[k] >= mb_area_start[a2 + 1]; a2++) > + for (a2 = a + 1; a2 < 4 && b->next[k] >= mb_area_start[a2 + 1]; a2++) > b->prev[a2] = prev; > av_assert2(a2 < 4); hmm, i cant really remember this clearly but from looking at the code it looks like this is the logic: b->next[k] < 64 b->next[k] >= mb_area_start[a + 1] implies mb_area_start[a + 1] < 64 which implies a < 3 and a2 < 4 on the first iteration so the first is still in the array subsequently, b->next[k] >= mb_area_start[a2 + 1] exists before the end as b->next[k] < 64 and the last entry being 64 [...]
On 11/17/2017 5:37 PM, Michael Niedermayer wrote: > hmm, i cant really remember this clearly but from looking at the code > it looks like this is the logic: > b->next[k] < 64 > b->next[k] >= mb_area_start[a + 1] implies mb_area_start[a + 1] < 64 > which implies a < 3 > and a2 < 4 on the first iteration so the first is still in the array > subsequently, b->next[k] >= mb_area_start[a2 + 1] exists before the end > as b->next[k] < 64 and the last entry being 64 Seems to be the case, though it is incredibly non-obvious. Is there a better way to have that assert run than to check we'd have already run the OOB access? - Derek
diff --git a/libavcodec/dvenc.c b/libavcodec/dvenc.c index ce2fc75daa..b79cbebb04 100644 --- a/libavcodec/dvenc.c +++ b/libavcodec/dvenc.c @@ -383,7 +383,7 @@ static inline void dv_guess_qnos(EncBlockInfo *blks, int *qnos) prev = k; } else { if (b->next[k] >= mb_area_start[a + 1] && b->next[k] < 64) { - for (a2 = a + 1; b->next[k] >= mb_area_start[a2 + 1]; a2++) + for (a2 = a + 1; a2 < 4 && b->next[k] >= mb_area_start[a2 + 1]; a2++) b->prev[a2] = prev; av_assert2(a2 < 4); av_assert2(b->mb[b->next[k]]);
mb_area_start has 5 entries, and 'a' is iterated through from 0 to 3. 'a2' is set to 'a + 1', and mb_area_start[a2 + 1] is accessed, so if a is 3, then we try to access mb_area_start[5]. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- I'm not 100% sure if this fix is /correct/, so hopefully someone knows the DV code... --- libavcodec/dvenc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)