[PATCH] golomb: check log validity before shifting

[Michael Niedermayer]

Fixes invalid right shift in fate-cavs

Signed-off-by: Michael Niedermayer <***@gmx.at>
---
libavcodec/golomb.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
index 0629c78..e3a35e9 100644
--- a/libavcodec/golomb.h
+++ b/libavcodec/golomb.h
@@ -66,10 +66,14 @@ static inline int get_ue_golomb(GetBitContext *gb){
return ff_ue_golomb_vlc_code[buf];
}else{
log= 2*av_log2(buf) - 31;
- buf>>= log;
- buf--;
LAST_SKIP_BITS(re, gb, 32 - log);
CLOSE_READER(re, gb);
+ if (log < 0) {
+ av_log(0, AV_LOG_ERROR, "Invalid UE golomb code\n");
+ return AVERROR_INVALIDDATA;
+ }
+ buf>>= log;
+ buf--;

return buf;
}

[Paul B Mahol]

Post by Michael Niedermayer
Fixes invalid right shift in fate-cavs
---
libavcodec/golomb.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
index 0629c78..e3a35e9 100644
--- a/libavcodec/golomb.h
+++ b/libavcodec/golomb.h
@@ -66,10 +66,14 @@ static inline int get_ue_golomb(GetBitContext *gb){
return ff_ue_golomb_vlc_code[buf];
}else{
log= 2*av_log2(buf) - 31;
- buf>>= log;
- buf--;
LAST_SKIP_BITS(re, gb, 32 - log);
CLOSE_READER(re, gb);
+ if (log < 0) {
+ av_log(0, AV_LOG_ERROR, "Invalid UE golomb code\n");
+ return AVERROR_INVALIDDATA;
+ }
+ buf>>= log;
+ buf--;
return buf;
}
--
1.7.9.5

I'm not sure about return code, most code that calls it never check value.

[Michael Niedermayer]

Post by Paul B Mahol

Post by Michael Niedermayer
Fixes invalid right shift in fate-cavs
---
libavcodec/golomb.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
index 0629c78..e3a35e9 100644
--- a/libavcodec/golomb.h
+++ b/libavcodec/golomb.h
@@ -66,10 +66,14 @@ static inline int get_ue_golomb(GetBitContext *gb){
return ff_ue_golomb_vlc_code[buf];
}else{
log= 2*av_log2(buf) - 31;
- buf>>= log;
- buf--;
LAST_SKIP_BITS(re, gb, 32 - log);
CLOSE_READER(re, gb);
+ if (log < 0) {
+ av_log(0, AV_LOG_ERROR, "Invalid UE golomb code\n");
+ return AVERROR_INVALIDDATA;
+ }
+ buf>>= log;
+ buf--;
return buf;
}
--
1.7.9.5

I'm not sure about return code, most code that calls it never check value.

thats true, do you see a reason this code would be worse than -1 or
0 ? (0 would not allow the condition to be detected ...)

[...]

[Paul B Mahol]

Post by Michael Niedermayer

Post by Paul B Mahol

Post by Michael Niedermayer
Fixes invalid right shift in fate-cavs
---
libavcodec/golomb.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
index 0629c78..e3a35e9 100644
--- a/libavcodec/golomb.h
+++ b/libavcodec/golomb.h
@@ -66,10 +66,14 @@ static inline int get_ue_golomb(GetBitContext *gb){
return ff_ue_golomb_vlc_code[buf];
}else{
log= 2*av_log2(buf) - 31;
- buf>>= log;
- buf--;
LAST_SKIP_BITS(re, gb, 32 - log);
CLOSE_READER(re, gb);
+ if (log < 0) {
+ av_log(0, AV_LOG_ERROR, "Invalid UE golomb code\n");
+ return AVERROR_INVALIDDATA;
+ }
+ buf>>= log;
+ buf--;
return buf;
}
--
1.7.9.5

I'm not sure about return code, most code that calls it never check value.

thats true, do you see a reason this code would be worse than -1 or
0 ? (0 would not allow the condition to be detected ...)

Whatever that really fix bugs and not just fixes one and leave others flying.

Post by Michael Niedermayer
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact

[Michael Niedermayer]

Post by Paul B Mahol

Post by Michael Niedermayer

Post by Paul B Mahol

Post by Michael Niedermayer
Fixes invalid right shift in fate-cavs
---
libavcodec/golomb.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
index 0629c78..e3a35e9 100644
--- a/libavcodec/golomb.h
+++ b/libavcodec/golomb.h
@@ -66,10 +66,14 @@ static inline int get_ue_golomb(GetBitContext *gb){
return ff_ue_golomb_vlc_code[buf];
}else{
log= 2*av_log2(buf) - 31;
- buf>>= log;
- buf--;
LAST_SKIP_BITS(re, gb, 32 - log);
CLOSE_READER(re, gb);
+ if (log < 0) {
+ av_log(0, AV_LOG_ERROR, "Invalid UE golomb code\n");
+ return AVERROR_INVALIDDATA;
+ }
+ buf>>= log;
+ buf--;
return buf;
}
--
1.7.9.5

I'm not sure about return code, most code that calls it never check value.

thats true, do you see a reason this code would be worse than -1 or
0 ? (0 would not allow the condition to be detected ...)

Whatever that really fix bugs and not just fixes one and leave others flying.

It should fix the IOC failure and makes the invalid cases more
vissible. It doesnt make callers check the return code. Later may
in some cases have speedloss implications if it where done

ill wait a day and if i hear no objections from anyone then ill commit

[...]

[Michael Niedermayer]

Post by Michael Niedermayer

Post by Paul B Mahol

Post by Michael Niedermayer

Post by Paul B Mahol

Post by Michael Niedermayer
Fixes invalid right shift in fate-cavs
---
libavcodec/golomb.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h
index 0629c78..e3a35e9 100644
--- a/libavcodec/golomb.h
+++ b/libavcodec/golomb.h
@@ -66,10 +66,14 @@ static inline int get_ue_golomb(GetBitContext *gb){
return ff_ue_golomb_vlc_code[buf];
}else{
log= 2*av_log2(buf) - 31;
- buf>>= log;
- buf--;
LAST_SKIP_BITS(re, gb, 32 - log);
CLOSE_READER(re, gb);
+ if (log < 0) {
+ av_log(0, AV_LOG_ERROR, "Invalid UE golomb code\n");
+ return AVERROR_INVALIDDATA;
+ }
+ buf>>= log;
+ buf--;
return buf;
}
--
1.7.9.5

I'm not sure about return code, most code that calls it never check value.

thats true, do you see a reason this code would be worse than -1 or
0 ? (0 would not allow the condition to be detected ...)

Whatever that really fix bugs and not just fixes one and leave others flying.

It should fix the IOC failure and makes the invalid cases more
vissible. It doesnt make callers check the return code. Later may
in some cases have speedloss implications if it where done
ill wait a day and if i hear no objections from anyone then ill commit

made it conditional on a numeric anomaly checker being enabled
and applied
maybe the check should always be done but that can be added later
ATM the only known issue is IOC failure and adding code may have
speed implications ...

[...]