[FFmpeg-devel] [PATCH 1/7] avcodec/snow_dwt: Fix left shifts of negative numbers

[FFmpeg-devel] [PATCH 1/7] avcodec/snow_dwt: Fix left shifts of negative numbers

[Andreas Rheinhardt]

Affected the vsynth(1|2|_lena)-snow(|-hpel) tests.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/snow_dwt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavcodec/snow_dwt.c b/libavcodec/snow_dwt.c
index 18b315ef66..965f409002 100644
--- a/libavcodec/snow_dwt.c
+++ b/libavcodec/snow_dwt.c
@@ -778,10 +778,10 @@ static inline int w_c(struct MpegEncContext *v, const uint8_t *pix1, const uint8
 
     for (i = 0; i < h; i++) {
         for (j = 0; j < w; j += 4) {
-            tmp[32 * i + j + 0] = (pix1[j + 0] - pix2[j + 0]) << 4;
-            tmp[32 * i + j + 1] = (pix1[j + 1] - pix2[j + 1]) << 4;
-            tmp[32 * i + j + 2] = (pix1[j + 2] - pix2[j + 2]) << 4;
-            tmp[32 * i + j + 3] = (pix1[j + 3] - pix2[j + 3]) << 4;
+            tmp[32 * i + j + 0] = (pix1[j + 0] - pix2[j + 0]) * (1 << 4);
+            tmp[32 * i + j + 1] = (pix1[j + 1] - pix2[j + 1]) * (1 << 4);
+            tmp[32 * i + j + 2] = (pix1[j + 2] - pix2[j + 2]) * (1 << 4);
+            tmp[32 * i + j + 3] = (pix1[j + 3] - pix2[j + 3]) * (1 << 4);
         }
         pix1 += line_size;
         pix2 += line_size;
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/7] avcodec/motion_est_template: Avoid using last + 1 element of array

[Andreas Rheinhardt]

For an int array[8][2] using &array[8][0] (which is an int*
pointing to the element beyond the last element of array)
triggers a "runtime error: index 8 out of bounds for type 'int[8][2]'"
from (Clang-)UBSan in the fate-vsynth(1|2|_lena)-snow tests.

I don't know whether this is really undefined behaviour or does not
actually fall under the "pointer arithmetic with the element beyond
the last element of the array is allowed as long as it is not
accessed" exception". All I know is that the code itself does not
read from beyond the last element of the array.

Nevertheless rewrite the code to a form that UBSan does not complain
about.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/motion_est_template.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/motion_est_template.c b/libavcodec/motion_est_template.c
index f3e94b7ebb..1888697db7 100644
--- a/libavcodec/motion_est_template.c
+++ b/libavcodec/motion_est_template.c
@@ -281,7 +281,7 @@ static int qpel_motion_search(MpegEncContext * s,
                     for(i=0; i<8; i++){
                         if(score < best[i]){
                             memmove(&best[i+1], &best[i], sizeof(int)*(7-i));
-                            memmove(&best_pos[i+1][0], &best_pos[i][0], sizeof(int)*2*(7-i));
+                            memmove(&best_pos[i + 1], &best_pos[i], sizeof(*best_pos) * (7 - i));
                             best[i]= score;
                             best_pos[i][0]= nx + 4*mx;
                             best_pos[i][1]= ny + 4*my;
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/7] avcodec/snowenc: Fix invalid left shift of negative numbers

[Andreas Rheinhardt]

Affected the vsynth(1|2|_lena)-snow(|-hpel) tests.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/snowenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
index ea0d4fc27f..ada24f7895 100644
--- a/libavcodec/snowenc.c
+++ b/libavcodec/snowenc.c
@@ -935,7 +935,7 @@ static av_always_inline int check_block_inter(SnowContext *s, int mb_x, int mb_y
     av_assert2(mb_x < b_stride);
 
     index = (p0 + 31 * p1) & (ME_CACHE_SIZE-1);
-    value = s->me_cache_generation + (p0 >> 10) + (p1 << 6) + (block->ref << 12);
+    value = s->me_cache_generation + (p0 >> 10) + p1 * (1 << 6) + (block->ref << 12);
     if (s->me_cache[index] == value)
         return 0;
     s->me_cache[index] = value;
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/7] avutil/aes: Don't use misaligned pointers

[Andreas Rheinhardt]

The AES code uses av_aes_block, a union consisting of
uint64_t[2], uint32_t[4], uint8_t[4][4] and uint8_t[16].
subshift() performs byte-wise manipulations of two av_aes_blocks,
but when encrypting, it does so with a shift of two bytes;
more precisely, it uses
"av_aes_block *s1 = (av_aes_block *) (s0[0].u8 - s)"
and lateron uses the uint8_t[16] member to access s0.
Yet av_aes_block requires to be suitably aligned for
the uint64_t[2] member, which s0[0].u8 - 2 is certainly
not. This is in violation of 6.3.2.3 (7) of C11. UBSan
reports this in the aes_ctr, mov-3elist-encrypted,
mov-frag-encrypted, mov-tenc-only-encrypted and srtp
tests.
Furthermore, there is another issue here: The pointer points
outside of s0; this works, because all the accesses lateron
use an index >= 3. (Clang-)UBSan reports this as
"runtime error: index -2 out of bounds for type 'uint8_t[16]'".

This commit fixes both of these issues: The latter issue
is fixed by applying an offset of "+ 3" during the cast
and subtracting this from the indices used lateron.
The former issue is solved by not casting to av_aes_block*
at all; instead simply cast to unsigned char*.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/aes.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/libavutil/aes.c b/libavutil/aes.c
index 029d738f87..8b78daa782 100644
--- a/libavutil/aes.c
+++ b/libavutil/aes.c
@@ -80,25 +80,27 @@ static inline void addkey_d(uint8_t *dst, const av_aes_block *src,
 
 static void subshift(av_aes_block s0[2], int s, const uint8_t *box)
 {
-    av_aes_block *s1 = (av_aes_block *) (s0[0].u8 - s);
-    av_aes_block *s3 = (av_aes_block *) (s0[0].u8 + s);
+    unsigned char *s1_dst = (unsigned char*)s0[0].u8 + 3 - s;
+    const unsigned char *s1_src = s1_dst + sizeof(*s0);
+    unsigned char *s3_dst = (unsigned char*)s0[0].u8 + s + 1;
+    const unsigned char *s3_src = s3_dst + sizeof(*s0);
 
     s0[0].u8[ 0] = box[s0[1].u8[ 0]];
     s0[0].u8[ 4] = box[s0[1].u8[ 4]];
     s0[0].u8[ 8] = box[s0[1].u8[ 8]];
     s0[0].u8[12] = box[s0[1].u8[12]];
-    s1[0].u8[ 3] = box[s1[1].u8[ 7]];
-    s1[0].u8[ 7] = box[s1[1].u8[11]];
-    s1[0].u8[11] = box[s1[1].u8[15]];
-    s1[0].u8[15] = box[s1[1].u8[ 3]];
+    s1_dst[ 0] = box[s1_src[ 4]];
+    s1_dst[ 4] = box[s1_src[ 8]];
+    s1_dst[ 8] = box[s1_src[12]];
+    s1_dst[12] = box[s1_src[ 0]];
     s0[0].u8[ 2] = box[s0[1].u8[10]];
     s0[0].u8[10] = box[s0[1].u8[ 2]];
     s0[0].u8[ 6] = box[s0[1].u8[14]];
     s0[0].u8[14] = box[s0[1].u8[ 6]];
-    s3[0].u8[ 1] = box[s3[1].u8[13]];
-    s3[0].u8[13] = box[s3[1].u8[ 9]];
-    s3[0].u8[ 9] = box[s3[1].u8[ 5]];
-    s3[0].u8[ 5] = box[s3[1].u8[ 1]];
+    s3_dst[ 0] = box[s3_src[12]];
+    s3_dst[12] = box[s3_src[ 8]];
+    s3_dst[ 8] = box[s3_src[ 4]];
+    s3_dst[ 4] = box[s3_src[ 0]];
 }
 
 static inline int mix_core(uint32_t multbl[][256], int a, int b, int c, int d)
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 5/7] avutil/aes: Don't use out-of-bounds index

[Andreas Rheinhardt]

Up until now, av_aes_init() uses a->round_key[0].u8 + t
as dst of memcpy where it is intended for t to be greater
than 16 (u8 is an uint8_t[16]); given that round_key itself
is an array, it is actually intended for dst to be
in a latter round_key member. To do this properly,
just cast a->round_key to unsigned char*.

This fixes the srtp, aes, aes_ctr, mov-3elist-encrypted,
mov-frag-encrypted and mov-tenc-only-encrypted
FATE-tests with (Clang-)UBSan.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/aes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/aes.c b/libavutil/aes.c
index 8b78daa782..2f08fb4164 100644
--- a/libavutil/aes.c
+++ b/libavutil/aes.c
@@ -253,7 +253,7 @@ int av_aes_init(AVAES *a, const uint8_t *key, int key_bits, int decrypt)
                     tk[j][i] ^= sbox[tk[j - 1][i]];
         }
 
-        memcpy(a->round_key[0].u8 + t, tk, KC * 4);
+        memcpy((unsigned char*)a->round_key + t, tk, KC * 4);
     }
 
     if (decrypt) {
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 6/7] avutil/integer: Fix undefined left shifts of negative numbers

[Andreas Rheinhardt]

Affected the integers FATE-test.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/integer.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/libavutil/integer.c b/libavutil/integer.c
index b709c6d487..a692c3783c 100644
--- a/libavutil/integer.c
+++ b/libavutil/integer.c
@@ -103,7 +103,7 @@ AVInteger av_shr_i(AVInteger a, int s){
     for(i=0; i<AV_INTEGER_SIZE; i++){
         unsigned int index= i + (s>>4);
         unsigned int v=0;
-        if(index+1<AV_INTEGER_SIZE) v = a.v[index+1]<<16;
+        if (index + 1 < AV_INTEGER_SIZE) v  = a.v[index + 1] * (1U << 16);
         if(index  <AV_INTEGER_SIZE) v+= a.v[index  ];
         out.v[i]= v >> (s&15);
     }
@@ -158,11 +158,9 @@ AVInteger av_int2i(int64_t a){
 }
 
 int64_t av_i2int(AVInteger a){
-    int i;
-    int64_t out=(int8_t)a.v[AV_INTEGER_SIZE-1];
+    uint64_t out = 0;
 
-    for(i= AV_INTEGER_SIZE-2; i>=0; i--){
+    for (int i = 3; i >= 0; i--)
         out = (out<<16) + a.v[i];
-    }
     return out;
 }
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 7/7] avutil/integer: Use '|' instead of '+' where it is more natural

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavutil/integer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavutil/integer.c b/libavutil/integer.c
index a692c3783c..d0a0f62c9a 100644
--- a/libavutil/integer.c
+++ b/libavutil/integer.c
@@ -104,7 +104,7 @@ AVInteger av_shr_i(AVInteger a, int s){
         unsigned int index= i + (s>>4);
         unsigned int v=0;
         if (index + 1 < AV_INTEGER_SIZE) v  = a.v[index + 1] * (1U << 16);
-        if(index  <AV_INTEGER_SIZE) v+= a.v[index  ];
+        if (index     < AV_INTEGER_SIZE) v |= a.v[index];
         out.v[i]= v >> (s&15);
     }
     return out;
@@ -161,6 +161,6 @@ int64_t av_i2int(AVInteger a){
     uint64_t out = 0;
 
     for (int i = 3; i >= 0; i--)
-        out = (out<<16) + a.v[i];
+        out = (out << 16) | a.v[i];
     return out;
 }
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/7] avcodec/snowenc: Fix invalid left shift of negative numbers

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 492 bytes --]

On Fri, Oct 21, 2022 at 08:59:35PM +0200, Andreas Rheinhardt wrote:
> Affected the vsynth(1|2|_lena)-snow(|-hpel) tests.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/snowenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

LGTM

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is a danger to trust the dream we wish for rather than
the science we have, -- Dr. Kenneth Brown

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/7] avcodec/snow_dwt: Fix left shifts of negative numbers

[Andreas Rheinhardt]

Andreas Rheinhardt:
> Affected the vsynth(1|2|_lena)-snow(|-hpel) tests.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/snow_dwt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/snow_dwt.c b/libavcodec/snow_dwt.c
> index 18b315ef66..965f409002 100644
> --- a/libavcodec/snow_dwt.c
> +++ b/libavcodec/snow_dwt.c
> @@ -778,10 +778,10 @@ static inline int w_c(struct MpegEncContext *v, const uint8_t *pix1, const uint8
>  
>      for (i = 0; i < h; i++) {
>          for (j = 0; j < w; j += 4) {
> -            tmp[32 * i + j + 0] = (pix1[j + 0] - pix2[j + 0]) << 4;
> -            tmp[32 * i + j + 1] = (pix1[j + 1] - pix2[j + 1]) << 4;
> -            tmp[32 * i + j + 2] = (pix1[j + 2] - pix2[j + 2]) << 4;
> -            tmp[32 * i + j + 3] = (pix1[j + 3] - pix2[j + 3]) << 4;
> +            tmp[32 * i + j + 0] = (pix1[j + 0] - pix2[j + 0]) * (1 << 4);
> +            tmp[32 * i + j + 1] = (pix1[j + 1] - pix2[j + 1]) * (1 << 4);
> +            tmp[32 * i + j + 2] = (pix1[j + 2] - pix2[j + 2]) * (1 << 4);
> +            tmp[32 * i + j + 3] = (pix1[j + 3] - pix2[j + 3]) * (1 << 4);
>          }
>          pix1 += line_size;
>          pix2 += line_size;

Will apply the remaining patches of this patchset tomorrow unless there
are objections.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".