Skip to content

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

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 12, 2023
Merged

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

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

jaromil
jaromil reviewed May 12, 2023
View reviewed changes
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaromil jaromil jaromil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

heap buffer overflow in filter curves
3 participants
@rrrapha @j-b-m @jaromil