marin-m / SongRec

An open-source Shazam client for Linux, written in Rust.
GNU General Public License v3.0
1.39k stars 103 forks source link

Program panics with integer overflow when making audio fingerprints #159

Closed 0xNF closed 7 months ago

0xNF commented 7 months ago

The latest master branch panics with integer overflow during the do_peak_recognition step of fingerprinting.

Using ubuntu on WSL2. ffmpeg is installed.

System Info ```bash $ lsb_release -a Distributor ID: Ubuntu Description: Ubuntu 20.04 LTS Release: 20.04 Codename: focal ``` ```bash $ uname -a Linux CompName 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux ``` ```bash $ ffmpeg -version ffmpeg version 4.2.7-0ubuntu0.1 Copyright (c) 2000-2022 the FFmpeg developers built with gcc 9 (Ubuntu 9.4.0-1ubuntu1~20.04.1) configuration: --prefix=/usr --extra-version=0ubuntu0.1 --toolchain=hardened --libdir=/usr/lib/x86_64-linux-gnu --incdir=/usr/include/x86_64-linux-gnu --arch=amd64 --enable-gpl --disable-stripping --enable-avresample --disable-filter=resample --enable-avisynth --enable-gnutls --enable-ladspa --enable-libaom --enable-libass --enable-libbluray --enable-libbs2b --enable-libcaca --enable-libcdio --enable-libcodec2 --enable-libflite --enable-libfontconfig --enable-libfreetype --enable-libfribidi --enable-libgme --enable-libgsm --enable-libjack --enable-libmp3lame --enable-libmysofa --enable-libopenjpeg --enable-libopenmpt --enable-libopus --enable-libpulse --enable-librsvg --enable-librubberband --enable-libshine --enable-libsnappy --enable-libsoxr --enable-libspeex --enable-libssh --enable-libtheora --enable-libtwolame --enable-libvidstab --enable-libvorbis --enable-libvpx --enable-libwavpack --enable-libwebp --enable-libx265 --enable-libxml2 --enable-libxvid --enable-libzmq --enable-libzvbi --enable-lv2 --enable-omx --enable-openal --enable-opencl --enable-opengl --enable-sdl2 --enable-libdc1394 --enable-libdrm --enable-libiec61883 --enable-nvenc --enable-chromaprint --enable-frei0r --enable-libx264 --enable-shared libavutil 56. 31.100 / 56. 31.100 libavcodec 58. 54.100 / 58. 54.100 libavformat 58. 29.100 / 58. 29.100 libavdevice 58. 8.100 / 58. 8.100 libavfilter 7. 57.100 / 7. 57.100 libavresample 4. 0. 0 / 4. 0. 0 libswscale 5. 5.100 / 5. 5.100 libswresample 3. 5.100 / 3. 5.100 libpostproc 55. 5.100 / 55. 5.100 ```
$ ./target/debug/songrec audio-file-to-fingerprint ./1975_newgrounds_jermai.mp3
thread 'main' panicked at 'attempt to add with overflow', src/fingerprinting/algorithm.rs:255:29
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The line it crashes at is

let corrected_peak_frequency_bin: u16 = 
    bin_position as u16 * 64 + (peak_variation_2 as i16) as u16;

Some of the variable values are:

bin_position = 669;
peak_variation_2 = -13.6644182;

Running these values through the castings and additions, we end up with

let corrected_peak_frequency_bin = (bin_position as u16) * 64 + ((peak_variation_2 as i16) as u16);
// 42816 + 41333= 84149

which of course is greater than a u16.

I'm working with the mp3 file from https://www.newgrounds.com/audio/download/1975, but it happens with every song I run through.

marin-m commented 7 months ago

Hello,

There was a recent change in this part of the code (change https://github.com/marin-m/SongRec/commit/d40f1840aceb432c455fffef8b298cd68654ae84 performed after issue https://github.com/marin-m/SongRec/issues/143), handling specifically the case of adding a negative peak_variation variable.

In this change, I introduced an intentional rollover integer overflow behavior (as I would do in C to add up a large unsigned short and a small signed short).

Indeed, this was a stupid thing to do because the runtime integer overflow arithmetic checks are disabled only in the release mode in Rust. In debug mode (as you are compiling with: ./target/debug/), voluntary integer overflows will be checked and provoke crash.

Hence thank you for noticing, I introduced a fix in commit 766a999

However beware, SongRec compiled in debug mode is very slow to recognize songs.

Regards,

0xNF commented 7 months ago

Thanks for the swift fix. I thought about just moving to i32s myself but I didn't know if it would cause any issues, since I'm thoroughly unqualified to judge the output.

And thanks for the warning about debug mode. I'm just using it to explore use cases as a library so I'm unconcerned with speed right now, but I'll keep that in mind.