diff mbox series

[FFmpeg-devel,3/4] avfilter/asrc_sine: Fix invalid left shift of negative number

Message ID 20200122145210.6898-3-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/4] avcodec/startcode: Use AV_RN due to UBSan warning about unaligned access
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 22, 2020, 2:52 p.m. UTC
by using a multiplication instead. The multiplication can never overflow
an int because the sin-factor is only an int16_t.

Affected the FATE-tests filter-concat and filter-concat-vfr.

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

Comments

Nicolas George Jan. 22, 2020, 5:50 p.m. UTC | #1
Andreas Rheinhardt (12020-01-22):
> by using a multiplication instead. The multiplication can never overflow
> an int because the sin-factor is only an int16_t.

No objection.

> Affected the FATE-tests filter-concat and filter-concat-vfr.

I find that suspect, the change is only theoretical. What architecture
are giving this?

Regards,
Nicolas George Jan. 22, 2020, 6 p.m. UTC | #2
Nicolas George (12020-01-22):
> Andreas Rheinhardt (12020-01-22):
> > Clang with UBSan. It of course works in practice, but we don't know whether
> > future compilers might exploit undefined behaviour in an unintended way, so
> > these theoretical issues should be fixed.
> 
> This one is rather clear-cut. Although I would very much appreciate an
> excerpt of the code before and after.
> 
> But for the future, we must remember that the difference between
> undefined behavior and implementation defined behavior is only relevant
> to compiler developers. FFmpeg is not a compiler: all behaviors are
> implementation defined. It may make a difference later for
> performance-critical cases.

Please remember to reply to the list. I don't know what software you are
using; for good software it should be the default.

Regards,
diff mbox series

Patch

diff --git a/libavfilter/asrc_sine.c b/libavfilter/asrc_sine.c
index 3a87210b4b..61499ee2cd 100644
--- a/libavfilter/asrc_sine.c
+++ b/libavfilter/asrc_sine.c
@@ -247,7 +247,7 @@  static int request_frame(AVFilterLink *outlink)
         samples[i] = sine->sin[sine->phi >> (32 - LOG_PERIOD)];
         sine->phi += sine->dphi;
         if (sine->beep_index < sine->beep_length) {
-            samples[i] += sine->sin[sine->phi_beep >> (32 - LOG_PERIOD)] << 1;
+            samples[i] += sine->sin[sine->phi_beep >> (32 - LOG_PERIOD)] * 2;
             sine->phi_beep += sine->dphi_beep;
         }
         if (++sine->beep_index == sine->beep_period)