[PATCH] run_event_on_dir: fix double free

[Martin Milata]

Run_event_on_dir freed the message buffer even if the logging callback
took ownership. Fix that.
---
 src/lib/run_event.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/src/lib/run_event.c b/src/lib/run_event.c
index ba9920c..6f31884 100644
--- a/src/lib/run_event.c
+++ b/src/lib/run_event.c
@@ -512,7 +512,14 @@ int run_event_on_dir_name(struct run_event_state *state,
             }
             /* no special prefix -> forward to log if applicable */
             else if (state->logging_callback)
+            {
                 msg = state->logging_callback(msg, state->logging_param);
+
+                /* callback took ownership of the buffer - don't free it */
+                if (!msg)
+                    buf = NULL;
+            }
+
             free(buf);
         }
         fclose(fp); /* Got EOF, close. This also closes state->command_out_fd */

[Jiri Moskovcak]

On 07/21/2011 12:32 PM, Martin Milata wrote:
> Run_event_on_dir freed the message buffer even if the logging callback
> took ownership. Fix that.
>
> src/lib/run_event.c |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/src/lib/run_event.c b/src/lib/run_event.c
> index ba9920c..6f31884 100644
> --- a/src/lib/run_event.c
> +++ b/src/lib/run_event.c
> @@ -512,7 +512,14 @@ int run_event_on_dir_name(struct run_event_state *state,
>               }
>               /* no special prefix ->  forward to log if applicable */
>               else if (state->logging_callback)
>        {
>             msg = state->logging_callback(msg, state->logging_param);
>            /* callback took ownership of the buffer - don't free it */
>            if (!msg)
>                buf = NULL;
>        }
>         free(buf);
>     }
>     fclose(fp); /* Got EOF, close. This also closes state->command_out_fd */

I'm not sure if this is the correct fix. If the loggin_callback doesn't 
free the msg, then it leaks which is probably better than double free, 
but maybe we should just make sure logging_callback doesn't free the msg 
(which I think it shouldn't do) and just free it in the 
run_event_on_dir_name... Ideas?

J.

[Jiri Moskovcak]

On 07/21/2011 01:48 PM, Jiri Moskovcak wrote:
> On 07/21/2011 12:32 PM, Martin Milata wrote:
> > Run_event_on_dir freed the message buffer even if the logging callback
> > took ownership. Fix that.
> >
> > src/lib/run_event.c |    7 +++++++
> >    1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/src/lib/run_event.c b/src/lib/run_event.c
> > index ba9920c..6f31884 100644
> > --- a/src/lib/run_event.c
> > +++ b/src/lib/run_event.c
> > @@ -512,7 +512,14 @@ int run_event_on_dir_name(struct run_event_state *state,
> >                }
> >                /* no special prefix ->   forward to log if applicable */
> >                else if (state->logging_callback)
> >        {
> >              msg = state->logging_callback(msg, state->logging_param);
> >            /* callback took ownership of the buffer - don't free it */
> >            if (!msg)
> >                buf = NULL;
> >        }
> >          free(buf);
> >      }
> >      fclose(fp); /* Got EOF, close. This also closes state->command_out_fd */
>
> I'm not sure if this is the correct fix. If the loggin_callback doesn't
> free the msg, then it leaks which is probably better than double free,
> but maybe we should just make sure logging_callback doesn't free the msg
> (which I think it shouldn't do) and just free it in the
> run_event_on_dir_name... Ideas?
>
> J.

from the run_event.h:

<snip>
     /* Can take ownership of log_line, which is malloced. In this case, 
return NULL.
      * Otherwise should return log_line (it will be freed by caller)
      */
     char* (*logging_callback)(char *log_line, void *param);
</snip>

- so the fix is correct, please push it
> Crash-catcher mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/crash-catcher

[Michal Toman]

On 21.07.2011 13:48, Jiri Moskovcak wrote:
> On 07/21/2011 12:32 PM, Martin Milata wrote:
> > Run_event_on_dir freed the message buffer even if the logging callback
> > took ownership. Fix that.
> >
> > src/lib/run_event.c |    7 +++++++
> >    1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/src/lib/run_event.c b/src/lib/run_event.c
> > index ba9920c..6f31884 100644
> > --- a/src/lib/run_event.c
> > +++ b/src/lib/run_event.c
> > @@ -512,7 +512,14 @@ int run_event_on_dir_name(struct run_event_state *state,
> >                }
> >                /* no special prefix ->   forward to log if applicable */
> >                else if (state->logging_callback)
> >        {
> >              msg = state->logging_callback(msg, state->logging_param);
> >            /* callback took ownership of the buffer - don't free it */
> >            if (!msg)
> >                buf = NULL;
> >        }
> >          free(buf);
> >      }
> >      fclose(fp); /* Got EOF, close. This also closes state->command_out_fd */
>
> I'm not sure if this is the correct fix. If the loggin_callback doesn't
> free the msg, then it leaks which is probably better than double free,
> but maybe we should just make sure logging_callback doesn't free the msg
> (which I think it shouldn't do) and just free it in the
> run_event_on_dir_name... Ideas?
>
> J.

The logging_callback just takes a message - it should not care wheter it 
to call free or not. Anyway free must be called on *buf* variable in 
run_event_on_dir_name. buf != msg if the row is prefixed. Example:
ASK_YES_NO Are you sure?
^          ^
buf        msg

On the other hand, because of current if/else conditions prefixed 
messages are not passed to the logging_callback, however it can happen 
in the future (or is this too much paranoia?).

M
> Crash-catcher mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/crash-catcher