Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix a memory leak and an off-by-one problem in curves filter. #164

Merged
merged 2 commits into from
May 12, 2023

Conversation

rrrapha
Copy link
Contributor

@rrrapha rrrapha commented May 5, 2023

Not sure if this resolves #156. At least it prevents a segfault in kdenlive for me.

@j-b-m
Copy link
Contributor

j-b-m commented May 5, 2023

This patch fixes the MLT/Kdenlive crash on Bézier Curves.
I think the problem is with the size of the position *curve array, because after that we have a loop using steps of 1/c, which can easily go beyond the array length.

diff --git a/src/filter/curves/curves.c b/src/filter/curves/curves.c
index 7dac2be..f1b9e75 100644
--- a/src/filter/curves/curves.c
+++ b/src/filter/curves/curves.c
@@ -666,7 +666,7 @@ void updateBsplineMap(f0r_instance_t instance)
             c = 1;
         }
         step = 1 / (double)c;
-        position *curve = (position *) malloc(c * sizeof(position));
+        position *curve = (position *) malloc((c + 1) * sizeof(position));
         while (t <= 1) {
             curve[pn++] = pointOnBezier(t, p);
             t += step;

@rrrapha
Copy link
Contributor Author

rrrapha commented May 5, 2023

@j-b-m Yes, I think you are right. I have force-pushed this change.

@j-b-m
Copy link
Contributor

j-b-m commented May 11, 2023

@jaromil Any feedback on this ? It would be great to fix #156 since it prevents us to use the latest Frei0r version in Kdenlive. Thanks

@jaromil
Copy link
Member

jaromil commented May 11, 2023

@j-b-m sure, planning to re-run in address-sanitizer within this week

Copy link
Member

@jaromil jaromil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I detect an heap-buffer-overflow when running without a set parameter, this is due to the initialization of c/bsplineMap buffer to a single double as placeholder on filter construct.

==12188==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000013fc0 at pc 0x7fb558a1b5cb bp 0x7ffc55c11230 sp 0x7ffc55c11228
READ of size 8 at 0x602000013fc0 thread T0
    #0 0x7fb558a1b5ca in f0r_update /home/jrml/devel/frei0r/src/filter/curves/curves.c:812
    #1 0x5654e913f34a in main /home/jrml/devel/frei0r/test/frei0r-test.c:148
    #2 0x7fb570446189 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #3 0x7fb570446244 in __libc_start_main_impl ../csu/libc-start.c:381
    #4 0x5654e913e440 in _start (/home/jrml/devel/frei0r/test/frei0r-test+0x4440)

0x602000013fc0 is located 168 bytes to the right of 8-byte region [0x602000013f10,0x602000013f18)

@jaromil
Copy link
Member

jaromil commented May 12, 2023

The solution is to improve update with a detection of correct initialization, which happens only when a parameter is changes. Will commit my fix to this PR.

@jaromil jaromil merged commit c0bbc2f into dyne:master May 12, 2023
@rrrapha rrrapha deleted the curves branch May 12, 2023 07:49
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

heap buffer overflow in filter curves
3 participants