fix NULL permitted for out and fx pointer buffer in fluid_synth_process_LOCAL() #659

jjceresa · 2020-08-06T15:37:08Z

This should avoid a segmentation fault if one pass NULL value for fx or out parameters.

derselbst · 2020-08-08T10:06:44Z

Passing NULL as buffer while at the same time specifying nout>0 seems more like a bug (or an abuse) to me. This condition should better be handled as fluid_return_val_if_fail by returning FLUID_FAILED.

Apart from that, I really like the comments :)

jjceresa · 2020-08-08T19:10:50Z

Passing NULL as buffer while at the same time specifying nout>0 seems more like a bug (or an abuse) to me.

yes, it is more appropriate to return FLUID_FAILED to the caller.

jjceresa · 2020-08-08T19:16:20Z

Please, because of the xml-like notation in fluid_synth_process's comment header, I'm not sure if comment addition were correctly placed. Could you look this ?

derselbst · 2020-08-09T05:11:20Z

src/synth/fluid_synth.c

+    /*
+      Now len is > 0. This will lead in rendering in internal mixer buffer.
+      However this rendering must not occur if both nfx and nout are set to 0.
+      So now we check that both nfx and nout set to 0 while len > 0, is invalid.
+    */
+    fluid_return_val_if_fail(nfx || nout, FLUID_FAILED);


This change is incorrect and breaks the unit test. If len>0 while all buffers are NULL, rvoice_mixer will still render len samples, but nothing will be mixed.

derselbst · 2020-08-09T05:23:02Z

src/synth/fluid_synth.c

    fluid_return_val_if_fail(nfx % 2 == 0, FLUID_FAILED);
+
+    /* out NULL while nout > 0 is invalid */
+    fluid_return_val_if_fail((fx != NULL) || (nfx == 0), FLUID_FAILED);


Copy paste mistake

Oups !, thanks

derselbst · 2020-08-09T05:25:45Z

src/synth/fluid_synth.c

+ * and in the range <code>0 <= nfx/2 <= (fluid_synth_count_effects_channels() * fluid_synth_count_effects_groups()).
+  Note that zero value is valid and allows to skip mixing effects in all fx output buffers.</code>.


Suggested change

* and in the range <code>0 <= nfx/2 <= (fluid_synth_count_effects_channels() * fluid_synth_count_effects_groups()).

Note that zero value is valid and allows to skip mixing effects in all fx output buffers.</code>.

* and in the range <code>0 <= nfx/2 <= (fluid_synth_count_effects_channels() * fluid_synth_count_effects_groups())</code>.

Note that zero value is valid and allows to skip mixing effects in all fx output buffers.

derselbst · 2020-08-09T05:27:04Z

src/synth/fluid_synth.c

+(because of stereo) and in the range <code>0 <= nout/2 <= fluid_synth_count_audio_channels().
+ Note that zero value is valid and allows to skip mixing dry audio in all out output buffers.</code>.


Suggested change

(because of stereo) and in the range <code>0 <= nout/2 <= fluid_synth_count_audio_channels().

Note that zero value is valid and allows to skip mixing dry audio in all out output buffers.</code>.

(because of stereo) and in the range <code>0 <= nout/2 <= fluid_synth_count_audio_channels()</code>.

Note that zero value is valid and allows to skip mixing dry audio in all out output buffers.

derselbst · 2020-08-09T05:32:04Z

src/synth/fluid_synth.c

+ *  - fx NULL while nfx > 0, or out NULL while nout > 0.
+ *  - nfx or nout not multiple of 2.
+ *  - len < 0.
+ *  - both nfx and nout set to 0 while len > 0
+ *  - nfx or nout greater than respective internal mixer buffer count.


Suggested change

* - fx NULL while nfx > 0, or out NULL while nout > 0.

* - nfx or nout not multiple of 2.

* - len < 0.

* - both nfx and nout set to 0 while len > 0

* - nfx or nout greater than respective internal mixer buffer count.

* - <code>fx == NULL</code> while <code>nfx > 0</code>, or <code>out == NULL</code> while <code>nout > 0</code>.

* - \c nfx or \c nout not multiple of 2.

* - <code>len < 0</code>.

* - \c nfx or \c nout exceed the range explained above.

Thanks. I assume that these xml-like notation will be read by a tool that produce formatted documentation in the Wiki, isn't it ?

The tool is called doxygen and generates the API doc and supports embedding HTML. Not to be confused with the wiki.

The tool is called doxygen and generates the API doc and supports embedding HTML.

Ok, thanks.

Not to be confused with the wiki.

Yes. In fact the wiki is only manually "generated".

-fix xml-like notation in comment. -fix out and nout parameter check. -allow internal rendering if len > 0 even if both nfx and nout are set to 0.

derselbst · 2020-08-10T18:37:44Z

Thanks!

* Properly handle overlapping notes when using fluid_event_note() (FluidSynth#637) * Fix regression introduced in a893994 Mentioned commit broke fluid_synth_start() when using a DLS soundfont. * Fix an uninitialized memory access that could possibly trigger an FPE trap for instruments that use the exclusive class generator * Update API docs * Bump to 2.1.4 * Update Doxyfile * Turn incompatible-pointer-types warning into error * Fix passing arguments from incompatible pointer type * Fix a NULL deref in jack driver * Fix a possible race condition during midi autoconnect * Fix printf format warnings * Update Android Asset loader to new callback API * Update Travis CI (FluidSynth#658) * update to Ubuntu Focal * use clang10 * avoid unintentional fallbacks to default `/usr/bin/c++` compiler * fix related compiler warnings * fix NULL permitted for out and fx pointer buffer Closes FluidSynth#659 * CMakeLists.txt: fix build with gcc 4.8 (FluidSynth#661) -Werror=incompatible-pointer-types is unconditionally used since version 2.1.4 and 137a14e. This will raise a build failure when checking for threads on gcc 4.8: /home/buildroot/autobuild/run/instance-3/output-1/host/bin/arm-none-linux-gnueabi-gcc --sysroot=/home/buildroot/autobuild/run/instance-3/output-1/host/arm-buildroot-linux-gnueabi/sysroot -DTESTKEYWORD=inline -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os -Wall -W -Wpointer-arith -Wcast-qual -Wstrict-prototypes -Wno-unused-parameter -Wdeclaration-after-statement -Werror=implicit-function-declaration -Werror=incompatible-pointer-types -Wbad-function-cast -Wcast-align -DNDEBUG -fPIE -o CMakeFiles/cmTC_98946.dir/CheckIncludeFile.c.o -c /home/buildroot/autobuild/run/instance-3/output-1/build/fluidsynth-2.1.4/CMakeFiles/CMakeTmp/CheckIncludeFile.c cc1: error: -Werror=incompatible-pointer-types: no option -Wincompatible-pointer-types Fixes: - http://autobuild.buildroot.org/results/13cbba871db56ef8657a3d13c6ac8e1b4da0d244 Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> * TravisCI: add a build for GCC 4.8 (FluidSynth#662) * Remove unused member variable * Limiting audio-channels to audio-groups (FluidSynth#663) * Use a runtime check to detect version of libinstpatch (FluidSynth#666) It could be that during runtime an older version of libinstpatch is used than the one fluidsynth was compiled against. In this case, libinstpatch will fail to load DLS fonts, because libinstpatch's initialization semantics don't match those compiled into fluidsynth. * Add a chart about voice mixing and rendering * Mapping of fx unit output to dry buffers in mix mode. (FluidSynth#668) Currently, all fx unit output (in mix mode) are mapped to the `first buffer`. This is not appropriate for synth.audio-groups > 1 This PR allows the mapping of fx output based on `fx unit index` and `synth.audio-groups` value. This allows us to get the `fx output `mixed to the respective `buffer` on which a `MIDI channel` is mapped. For example: with `synth.audio-groups = 3` and `synth.effect-groups = 3`: - MIDI chan 0 (dry + fx0) is mapped to buf 0 - MIDI chan 1 (dry + fx1) is mapped to buf 1 - MIDI chan 2 (dry + fx2) is mapped to buf 2 * Add multi channels support for audio driver. (FluidSynth#667) This PR addresses FluidSynth#665. 1) Add new functions for multi channels support: `fluid_synth_write_float_channels()`, `fluid_synth_write_s16_channels()` 2) `dsound` and `waveout` driver make use of this support. tested on 2 audio devices: - creative SB Live! (6 channels). - Realtek: ALC889A (8 channels). * Bump to 2.1.5 * Add SonarQube static code analysis (FluidSynth#671) * Add SonarQube and LGTM badges to README * Remove fluid_event_any_control_change() from public API (FluidSynth#674) Originally, I have only marked it deprecated. But since we have an SOVERSION bump next release and because this function was only meant for internal usage, I think it's safe to remove it right now. * Remove dead code * Fix an impossible NULL deref * Fix a NULL dereference Access to field 'zone' results in a dereference of a null pointer (loaded from variable 'prev_preset'), if `size` is negative. Problem is: Parameter `size` is `chunk.size` and should be unsigned. * Fix another NULL dereference Access to field 'zone' results in a dereference of a null pointer (loaded from variable 'pr'), if size is negative. However, size should be unsigned. * Remove a FIXME I don't see any problem calling fluid_channel_init() from within synth context * Remove a FIXME I don't see any 'allocation' of preset. And ALL public synth functions have a mutex lock which might potentially block when called from synth context, but only then if the client app pessimizes this situation by extensively calling the synth from outside the synth context. * Remove a FIXME I have no clue what it refers to or what it's meant by that. * Add comment into empty block * Remove a FIXME Not aware of any problems caused by the old glib thread API. It will be removed sooner or later anyway. * Remove a FIXME * Set the systemd unit target to default.target fluidsynth.service.in: The [Install] section [1] in systemd unit declares in which target the service will be started. The `multi-user.target` [2] - managed by the systemd _system_ service manager - is used in the `fluidsynth.service`. However, as it is a _user_ unit it needs to be pulled in by the `default.target` [3] instead, which is the main target for the user session (as started by `user@.service` [4]). [1] https://www.freedesktop.org/software/systemd/man/systemd.unit.html#%5BInstall%5D%20Section%20Options [2] https://www.freedesktop.org/software/systemd/man/systemd.special.html#multi-user.target [3] https://www.freedesktop.org/software/systemd/man/systemd.special.html#default.target1 [4] https://www.freedesktop.org/software/systemd/man/user@.service.html * Define FLUIDSYNTH_API on OS/2 Previously, CMake on OS/2 exported all the symbols unconditionally. Now it exports necessary symbols only. As a result, it's necessary to define FLUIDSYNTH_API correctly. Addresses FluidSynth#678 * Make winmidi driver multi devices capable. (FluidSynth#677) * Fix minor bug in windows audio driver (FluidSynth#680) * Improve error reporting in Waveout and DSound drivers * Fix Windows build * Add proper unicode support to Windows error reporting * Fix build on Windows 9x/ME Addresses FluidSynth#679 * Promote Controller/Pressure/Bend event functions to 32bits (FluidSynth#670) * Elaborate on synth.cpu-cores * Add FluidMixer chart to API docs * Ensure WaveOut compatibility with Win9X/NT (FluidSynth#687) * Update and rename README.Android.md to README.md * Update Android CircleCI build to use latest orb, Android API, Oboe and Cerbero (FluidSynth#690) This fixes the currently-broken CircleCI build for Android-useable .so files. Currently the Cerbero build is based off https://github.com/falrm/cerbero until https://gitlab.freedesktop.org/gstreamer/cerbero/-/merge_requests/641 is merged and deployed to the GitHub cerbero mirror. Here is a successful build with the updated CircleCI workflow: https://app.circleci.com/pipelines/github/falrm/fluidsynth-android/31/workflows/0ad3186a-394c-4736-984b-96496b608053/jobs/32 Fixes FluidSynth#688 * Replace FreeBSD 13.0 with 11.4 (FluidSynth#692) 13.0 hasn't been released yet and the CI build keeps failing for long. * Remove unused variable * Fix possible uninitialized use of dry_idx variable * avoid an unlikely race condition * Add hint message when compiled without getopt support (FluidSynth#697) * Add getopt support to CMake summary * Add public API to pin and unpin presets to the sample cache (FluidSynth#698) Following the discussion about an API to pin and unpin preset samples in the sample cache here: https://lists.nongnu.org/archive/html/fluid-dev/2020-10/msg00016.html Short explanation of the change: Only the default loader currently supports dynamic sample loading, so I thought it might be a good idea to keep the changes for this feature mostly contained in the default loader as well. I've added two new preset notify flags (FLUID_PRESET_PIN and FLUID_PRESET_UNPIN) that are handled by the preset->notify callback and trigger the loading and possibly unloading of the samples. * Revert "remove VintageDreamsWaves-v2.sf3" This reverts commit a36c06c. We've got explicit permission from Ian Wilson to convert it to SF3. Addresses FluidSynth#701. * Updated XSL / styling for fluidsettings.xml * Cleanup section label markup and rendering * Use (empty string) for empty default values of str settings * shell.port is an int setting, not num * Update periods and period-size with current values from source * Consistently format all floats * Better explain currently unused effects-channels * Update effects-groups description to avoid the word "unit" * Update ladspa.active description Use 1 (TRUE) for consistency and mention LADSPA documentation * As gs is default for midi-bank-select, list it as first option for clarity * Options seems to be more widely used, so use that instead of Choices * Remove FLUIDSYNTH_API and FLUID_DEPRECATED macros from documentation * Remove "References" and "Referenced by" links from doc They auto generated links are quite long on some functions, making the documentation harder to read. * Enable navigation sidebar * Make larger enums easier to read * Move doxygen customizations into separate directory * Restructure devdocs into separate pages * Change files into groups / modules * Some additional subgrouping * Use xsltproc to include settings in API documentation * Replace all links to fluidsettings.xml with proper \ref's * Command Shell group for all shell related commands With subgroups for command handler, shell and server. * Audio output group With subgroups Audio Driver and File Renderer * Logging interface * MIDI input group Contains MIDI Driver, MIDI Router, MIDI Player and MIDI Events * MIDI Seqencer documentation * Settings documentation * Miscellaneous group * SoundFont API Includes Generators, Modulators, Loader etc * Add version defines and functions to misc group * Rename setting reference page name to lowercase, for consistency * Structure the large synth header into subgroups Also include version.h and ladspa.h in the Synthesizer group. * Consistent capitalization of usage guide section names * Some more brief message abbreviation hints * Custom doxygen layout to rename modules to API Reference * Sort groups/modules, briefs and members * Updated documentation styling * Remove footer, as it takes away valuable vertical space * Make sure libxslt is only searched if doxygen is available as well * Also update the styling of the deprecated list * Mark settings with callbacks as realtime and output this in the generated docs * Separate new_* and delete_* functions from the rest * Sync the static Doxyfile with Doxyfile.cmake Still missing is the integration of the generated fluidsettings.txt, as that requires a build script currently not available on the server generating the public API docs. * Split doxygen INPUT into separate lines, for easier readability * Move recent changes into separate file * Move usage guide pages into separate files in doc/usage * Move examples into doc/examples directory * Split HTML_EXTRA_FILEs into separate lines * Use \image for images and improve quality of FluidMixer image * Use custom \setting{} alias to link to fluid settings * Smaller cleanup and reformatting of long lines. * Add generated fluidsettings.txt for fluidsynth.org API doc build Probably not the final solution, but works for now. * Hide nav sync toggle button * Style improvements for small screens - hide side nav - hide search box - make content full height * Improve styling of field tables (enum values) * Document how to revert the styling and layout changes * Add documentation hints to style guide * Make top links black on hover, not white * Add missing group brief descriptions * Remove debug leftover * Remove obsolete doxygen config options * Add intro text to deprecated list * Use SVG for fluid mixer image * Workaround for doxygen bug with linebreaks in ALIASES Using \_linebr is not ideal, as it's an internal command. But that seems to be the most compatible way to specify line breaks in ALIASES accross different doxygen versions at the moment. * GitHub Action to build the API docs from master branch (FluidSynth#704) Uploads the complete HTML API docs as an artifact called api_docs.zip * Remove unused command alias and sync Doxyfile.cmake and Doxyfile * Settings reference style more consistent with rest of reference pages * Update generated fluidsettings.txt for API doc build on fluidsynth.org * Fx unit api (FluidSynth#673) This PR addresses FluidSynth#669 point 2.1. It proposes set/get API functions to change/read fx unit parameters. The deprecated shell commands are updated. Now the commands line have 2 parameters: - first parameter is the fx unit index. - second parameter is the value to apply to the fx unit. * Update owner of the SoundFont registered trademark. (FluidSynth#706) As of the time of this PR, the SoundFont registered trademark is owned by Creative Technology Ltd. http://tmsearch.uspto.gov/bin/showfield?f=doc&state=4803:rj74xq.2.1 http://assignments.uspto.gov/assignments/q?db=tm&qt=sno&reel=&frame=&sno=74325965 * Handle GS SysEx messages for setting whether a channel is used for rhythm part. (FluidSynth#708) Some MIDI files that uses the GS standard uses channels other than channel 10 as percussion channel. Currently FluidSynth ignores the messages setting that up, causing notes meant to be played with a drum instrument played with a melodic instrument or vice versa. This patch will partially fix the issue. Currently the implementation in this patch doesn't cover a specific "quirk" in Roland GS Modules: they seem to remember the last used instrument in the selected mode. This patch simply sets the instrument number to 0 instead. A test file is attached. If played correctly (with `-o synth.device-id=16`) no out of place drum or piano sounds should be heard. [wikipedia_MIDI_sample_gstest.mid.gz](https://github.com/FluidSynth/fluidsynth/files/5610727/wikipedia_MIDI_sample_gstest.mid.gz) * Fix Windows CI Remove fake pkg-config * Re-enable unit tests with mingw and allow them to fail to ensure build artifacts are being published * Update API doc build to upload to GH pages * Fix build path in API doc publish step * Clean existing files in API doc on GH pages * Fix commit message for deploying API doc * Also set commit name and email for api doc build commits * Commit to test API doc build Will be removed with next commit again. * Revert "Commit to test API doc build" This reverts commit fd39f6e. * Make some strings const (FluidSynth#716) * Replace g_ascii_strtoll() with FLUID_STRTOL() (FluidSynth#717) * Elaborate on synth.device-id * Breaking unit tests for WindowsXP should be fatal * Update Issue templates to point to GitHub discussion Co-authored-by: Tom M <tom.mbrt@googlemail.com> Co-authored-by: jjceresa <jjc_fluid@orange.fr> Co-authored-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> Co-authored-by: jjceresa <32781294+jjceresa@users.noreply.github.com> Co-authored-by: David Runge <dave@sleepmap.de> Co-authored-by: KO Myung-Hun <komh@chollian.net> Co-authored-by: Jon Latané <jonlatane@gmail.com> Co-authored-by: Marcus Weseloh <marcus@weseloh.cc> Co-authored-by: Nathan Umali <some1namednate@gmail.com> Co-authored-by: Chris Xiong <chirs241097@gmail.com> Co-authored-by: Carlo Bramini <30959007+carlo-bramini@users.noreply.github.com>

Closes FluidSynth#659

fix NULL permitted for out and fx pointer buffer

119104a

jjceresa added 5 commits August 8, 2020 18:19

Refuse (out NULL,nout >0), (fx NULL,nfx > 0) parameters case

13d957e

avoid internal rendering if both nfx and nout are set to 0

b8f14d3

Add comment when function fail

82e9a8b

add comment about effects mixing output destination

38bfe33

add comment about public fluid_synth_process_LOCAL()

8be532b

derselbst reviewed Aug 9, 2020

View reviewed changes

jjceresa added 2 commits August 9, 2020 09:35

fix comment and parameters check.

f4224c2

-fix xml-like notation in comment. -fix out and nout parameter check. -allow internal rendering if len > 0 even if both nfx and nout are set to 0.

Add comment to fluid_synth_write_xxx() function

0fc98ba

derselbst closed this in 3ee2bbe Aug 10, 2020

derselbst deleted the fix-null-ptr branch August 10, 2020 18:38

jet2jet pushed a commit to jet2jet/fluidsynth-emscripten that referenced this pull request May 27, 2021

fix NULL permitted for out and fx pointer buffer

5d5faab

Closes FluidSynth#659

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix NULL permitted for out and fx pointer buffer in fluid_synth_process_LOCAL() #659

fix NULL permitted for out and fx pointer buffer in fluid_synth_process_LOCAL() #659

jjceresa commented Aug 6, 2020 •

edited

Loading

derselbst commented Aug 8, 2020

jjceresa commented Aug 8, 2020

jjceresa commented Aug 8, 2020

derselbst Aug 9, 2020

jjceresa Aug 9, 2020

derselbst Aug 9, 2020

jjceresa Aug 9, 2020

derselbst Aug 9, 2020

jjceresa Aug 9, 2020

derselbst Aug 9, 2020

derselbst Aug 9, 2020

jjceresa Aug 9, 2020

derselbst Aug 10, 2020 •

edited

Loading

jjceresa Aug 10, 2020

derselbst commented Aug 10, 2020

		* and in the range <code>0 <= nfx/2 <= (fluid_synth_count_effects_channels() * fluid_synth_count_effects_groups()).
		Note that zero value is valid and allows to skip mixing effects in all fx output buffers.</code>.

		(because of stereo) and in the range <code>0 <= nout/2 <= fluid_synth_count_audio_channels().
		Note that zero value is valid and allows to skip mixing dry audio in all out output buffers.</code>.

fix NULL permitted for out and fx pointer buffer in fluid_synth_process_LOCAL() #659

fix NULL permitted for out and fx pointer buffer in fluid_synth_process_LOCAL() #659

Conversation

jjceresa commented Aug 6, 2020 • edited Loading

derselbst commented Aug 8, 2020

jjceresa commented Aug 8, 2020

jjceresa commented Aug 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derselbst Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derselbst commented Aug 10, 2020

jjceresa commented Aug 6, 2020 •

edited

Loading

derselbst Aug 10, 2020 •

edited

Loading