Project

General

Profile

Bug #11336

Crash while drawing mask in exposure module

Added by Robert Hutton almost 3 years ago. Updated almost 3 years ago.

Status:
Fixed
Priority:
Low
Assignee:
-
Category:
Darkroom
Target version:
Start date:
11/27/2016
Due date:
% Done:

100%

Estimated time:
Affected Version:
2.2.0rc1
System:
Ubuntu
bitness:
64-bit
hardware architecture:
amd64/x86

Description

While drawing a mask using the path tool, in the exposure module.

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
0x00007fbe0bd2408b in __waitpid (pid=pid@entry=32025, stat_loc=stat_loc@entry=0x0, options=options@entry=0) at ../sysdeps/unix/sysv/linux/waitpid.c:29
29 ../sysdeps/unix/sysv/linux/waitpid.c: No such file or directory.
backtrace written to /tmp/darktable_bt_HX5ERY.txt
Segmentation fault (core dumped)

darktable_bt_HX5ERY.txt (30.3 KB) darktable_bt_HX5ERY.txt Robert Hutton, 11/27/2016 10:55 AM

Associated revisions

Revision 9e4f18dc (diff)
Added by Ulrich Pegelow almost 3 years ago

drawn masks: reconsider use of dt_masks_free_form() towards a fix for bug #11336

Revision 6e136c14 (diff)
Added by Ulrich Pegelow almost 3 years ago

drawn masks: reconsider use of dt_masks_free_form() towards a fix for bug #11336

(cherry picked from commit 9e4f18dca27fd56984b34706e929dae1dece6959)

History

#1 Updated by Roman Lebedev almost 3 years ago

Did you use new undo while working on this image?

#2 Updated by Robert Hutton almost 3 years ago

I don't think so; certainly not intentionally.

#3 Updated by Ulrich Pegelow almost 3 years ago

void dt_masks_free_form(dt_masks_form_t *form)
{
  if(!form) return;
  g_list_free_full(form->points, free);
  form->points = NULL;
  free(form);
  form = NULL;
  ^^^^^^^^^^^
}

That (^) doesn't look right. It only overwrites the argument in the called function, not (as it was supposedly intended) in the calling function. So we might get a double-free occasionally.

#4 Updated by Roman Lebedev almost 3 years ago

So far i was unable to reproduce this.
It might be related to #11276

#5 Updated by Ulrich Pegelow almost 3 years ago

So far i was unable to reproduce this. It might be related to #11276

Sounds like a different thing to me. No masks mentioned. Do you agree that dt_masks_free_form() looks buggy? (I'm a bit slow today and want to make sure that I am not completely wrong). If yes we anyhow need to fix that thing. Will take care.

#6 Updated by Roman Lebedev almost 3 years ago

Ulrich Pegelow wrote:

So far i was unable to reproduce this. It might be related to #11276

Sounds like a different thing to me. No masks mentioned.

Do you agree that dt_masks_free_form() looks buggy? (I'm a bit slow today and want to make sure that I am not completely wrong). If yes we anyhow need to fix that thing. Will take care.

That

form = NULL;

does indeed look misplaced.

Something like this semantic patch should be a better fit

@@
expression e;
@@
<...
 dt_masks_free_form(e);
+ e = NULL;
...>

#7 Updated by Ulrich Pegelow almost 3 years ago

Something like this semantic patch should be a better fit

Things might even be more complicated if one looks at path.c:dt_path_events_button_pressed() which is called by masks.c:dt_masks_events_button_pressed(). form is taken from darktable.develop->form_visible and potentially gets freed by using dt_masks_free_form(). Now all intermediate instances of form and the original storage place have to be set to NULL. This does not happen.

In order to fix we will need to use from by reference in any involved function calls which implies a lot of changes. IMHO that's too far reaching shortly before a release. I'm considering to only free form->points in dt_masks_free_form() and not free form itself, accepting the resulting memory leak.

#8 Updated by Roman Lebedev almost 3 years ago

Ulrich Pegelow wrote:

Something like this semantic patch should be a better fit

Things might even be more complicated if one looks at path.c:dt_path_events_button_pressed() which is called by masks.c:dt_masks_events_button_pressed(). form is taken from darktable.develop->form_visible and potentially gets freed by using dt_masks_free_form(). Now all intermediate instances of form and the original storage place have to be set to NULL. This does not happen.

In order to fix we will need to use from by reference in any involved function calls which implies a lot of changes. IMHO that's too far reaching shortly before a release.

All free() type functions take pointer that they should free, not ptr to pointer. Let's not invent wheel here at all, masks code is not nice as it is already :)

I'm considering to only free form->points in dt_masks_free_form() and not free form itself,

And then the function will no longer match it's name. Maybe this should completely be left for after 2.2

accepting the resulting memory leak.

uh

#9 Updated by Ulrich Pegelow almost 3 years ago

  • % Done changed from 0 to 10
  • Target version changed from 2.2.0 to Future
  • Status changed from New to Confirmed

Maybe this should completely be left for after 2.2

That's probably the best. So we leave fixing this bug for 2.3.

#10 Updated by Ulrich Pegelow almost 3 years ago

  • % Done changed from 10 to 100
  • Status changed from Confirmed to Fixed

#11 Updated by Roman Lebedev almost 3 years ago

  • Target version changed from Future to 2.4.0

Also available in: Atom PDF

Go to top