Potential null pointer fixes / checks #4132

OverOrion · 2022-09-13T13:31:53Z

In #4123 the fix was to check the return value of a failable function.
With the help of Infer I was able to find similar cases and fix them.

github-actions · 2022-09-13T13:32:14Z

No news file has been detected. Please write one, if applicable.

kira-syslogng · 2022-09-13T13:52:23Z

Build FAILURE

bazsi · 2022-09-13T14:16:04Z

lib/logreader.c

@@ -389,7 +389,7 @@ log_reader_work_finished(void *s)
      self->notify_code = 0;
      log_pipe_notify(self->control, notify_code, self);
    }
-  if (self->super.super.flags & PIF_INITIALIZED)
+  if ((self->super.super.flags & PIF_INITIALIZED) && self->proto)


this should rather be an assert in the body of the if.

self->proto should never be a NULL in this case (we had a bug in this path a while back in case of threaded(no)), and I reviewed that we have it covered. The bug at that time was caused by use-after-free.

So, I would rather get an abort in case self->proto is NULL here.

We concluded that self->proto being NULL here is allowed, as a pending_close can NULL it out a few lines above.

~~This was a rather naive approach, I blindly copied what I had found in LogWriter, thanks for catching it!~~

I do remember now, yes, self->proto can be NULL here.

bazsi · 2022-09-13T14:19:07Z

lib/control/control-command-thread.c

@@ -91,6 +91,7 @@ control_command_thread_run(ControlCommandThread *self)
  self->thread_finished.cookie = control_command_thread_ref(self);
  iv_event_register(&self->thread_finished);

+  g_assert(self->connection);


I don't see how this could occur. self->connection is initialized in the constructor by taking a ref from a constructor argument. The only call-site that calls control_command_thread_new() is control-connection which passes "self" to that argument.

I wouldn't want to spray unneeded asserts everywhere. If we want to add this anywhere, it should be in the constructor I think, but I don't think there's too much pressure doing it even there.

Indeed, it was a false positive, thanks once again for the catch!

bazsi · 2022-09-13T14:22:06Z

lib/logwriter.c

@@ -1201,7 +1199,7 @@ log_writer_write_message(LogWriter *self, LogMessage *msg, LogPathOptions *path_
            {
              if (!consumed)
                {
-                  g_free(self->line_buffer->str);
+                  g_string_free(self->line_buffer, TRUE);
                  log_writer_realloc_line_buffer(self);


this doubles the number of allocations as it will allocate both the GString struct and its "str" member. The aim was to optimize away this case.

A better alternative would be to use scratch-buffers for this purpose, or maybe wrap this ugly trick into a nicer looking function (aka g_string_steal).

Also, this seems to be a use-after-free, you are freeing self->line_buffer and then calling g_string_set_size() on it.

I gave it a second thought and refactored the function to take a ownership_transferred boolean which decides whether the old buffer should be freed or not. I think it helps with readability as the freeing would happen inside the function instead of the call side.

I am more than happy to revert it, because it does not contribute much to the spirit of the PR either.

bazsi · 2022-09-13T14:23:49Z

modules/affile/affile-dest.c

@@ -479,7 +479,6 @@ affile_dd_reuse_writer(gpointer key, gpointer value, gpointer user_data)
  affile_dw_set_owner(writer, self);
  if (!log_pipe_init(&writer->super))
    {
-      affile_dw_set_owner(writer, NULL);


I think removing this would cause a leak through the circular reference through the AFFileDestWriter->owner member.

I agree, affile_dw_set_owner() is necessary here, but its implementation needs to be fixed in order not to dereference NULL pointers. This is a real issue, we've reproduced the crash.

affile_dw_unset_owner() would be even cleaner.

Thanks once again, affile_dw_set_owner got fixed, it should take care of the possible NULL owner now.

bazsi

I've filed a few comments that should change before this can be merged.

MrAnno · 2022-10-01T14:09:21Z

@OverOrion Do you have plans to finish this PR?

Fix a potential null pointer dereference Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

Refactored this function to have it decied whether the old buffer should be freed or not. Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

MrAnno · 2023-05-07T12:36:52Z

Thanks for fixing these. I've reproduced one of the crashes, will review the PR tomorrow.

MrAnno · 2023-05-10T18:02:03Z

lib/logwriter.c

@@ -1203,17 +1206,15 @@ log_writer_write_message(LogWriter *self, LogMessage *msg, LogPathOptions *path_

      self->partial_write = (status == LPS_PARTIAL);

-      if (consumed)
-        log_writer_realloc_line_buffer(self);
+      log_writer_realloc_line_buffer(self, consumed);


This does not look like an equivalent transformation to me.
What's the idea behind this patch?

The idea was that the freeing was outside the other buffer manipulations and I wanted them to be close to each other (on the call site -> inside the function).

The consumed boolean is responsible for deciding whether a specific buffer has been passed yet or not. If it has been passed, then it should not be freed, because the reference is needed (there is something that refers to it).
If it has not been passed (nothing else refers to it), then the old buffer must be freed.

Basically I moved the g_free call into the function body.

MrAnno · 2023-05-10T18:04:02Z

modules/affile/affile-dest.c

@@ -292,24 +292,28 @@ static void
 affile_dw_set_owner(AFFileDestWriter *self, AFFileDestDriver *owner)
 {
  GlobalConfig *cfg = log_pipe_get_config(&owner->super.super.super);
+  log_pipe_set_config(&self->super, cfg);


This does not fix the NULL pointer deref issue as log_pipe_get_config dereferences owner.

You are right, I messed it up during the rebase 🤦 .

MrAnno · 2023-05-10T18:06:12Z

modules/affile/affile-dest.c

+{
+  if (self->owner)
+    log_pipe_unref(&self->owner->super.super.super);
+  self->owner = NULL;


Probably we should do something with self->super.expr_node too.

Upon a closer look it seems that the expr_node setting is done without freeing the old one in other places as well.

Should I fix it in this PR or in a separate one?

In very rare (and unrealistic) cases syslog-ng could crash if it was reloaded and it was unable to reuse it's file writers. Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

Signed-off-by: Szilard Parrag <szilard.parrag@gmail.com>

Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

bazsi reviewed Sep 13, 2022

View reviewed changes

bazsi requested changes Sep 13, 2022

View reviewed changes

MrAnno changed the title ~~Potential null pointer fixes / checks~~ WIP: Potential null pointer fixes / checks Dec 29, 2022

MrAnno marked this pull request as draft December 29, 2022 13:00

OverOrion added 3 commits May 7, 2023 14:05

lib: plugin

2b95c00

Fix a potential null pointer dereference Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

lib: make sure template is function is successfully retrieved

1c1cd95

Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

lib/logwriter: refactor log_writer_realloc_line_buffer

a5f24a7

Refactored this function to have it decied whether the old buffer should be freed or not. Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

OverOrion force-pushed the potential_null_ptr_fixes branch 2 times, most recently from bc13c3a to 834fd74 Compare May 7, 2023 12:29

MrAnno self-requested a review May 7, 2023 12:35

OverOrion force-pushed the potential_null_ptr_fixes branch from 834fd74 to 62f5e81 Compare May 7, 2023 12:49

OverOrion marked this pull request as ready for review May 7, 2023 12:50

OverOrion changed the title ~~WIP: Potential null pointer fixes / checks~~ Potential null pointer fixes / checks May 7, 2023

OverOrion force-pushed the potential_null_ptr_fixes branch from 2f77d1f to 1a139a1 Compare May 7, 2023 13:16

MrAnno requested changes May 10, 2023

View reviewed changes

OverOrion added 3 commits May 11, 2023 07:52

affile: remove null pointer dereference in affile_dd_reuse_writer()

060b13b

In very rare (and unrealistic) cases syslog-ng could crash if it was reloaded and it was unable to reuse it's file writers. Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

affile: add affile_dw_unset_owner

ce7b018

Signed-off-by: Szilard Parrag <szilard.parrag@gmail.com>

lib: fix possible null pointer dereference in logreader

1107829

Signed-off-by: Szilárd Parrag <szilard.parrag@gmail.com>

OverOrion force-pushed the potential_null_ptr_fixes branch from 1a139a1 to 1107829 Compare May 11, 2023 05:54

OverOrion requested a review from MrAnno May 25, 2023 07:31

MrAnno removed their request for review May 22, 2024 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Potential null pointer fixes / checks #4132

Potential null pointer fixes / checks #4132

OverOrion commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

kira-syslogng commented Sep 13, 2022

bazsi Sep 13, 2022

MrAnno Sep 18, 2022

OverOrion May 7, 2023 •

edited

bazsi Sep 13, 2022

OverOrion May 7, 2023

bazsi Sep 13, 2022

OverOrion May 7, 2023

bazsi Sep 13, 2022

MrAnno Sep 18, 2022

OverOrion May 7, 2023

bazsi left a comment

MrAnno commented Oct 1, 2022

MrAnno commented May 7, 2023

MrAnno May 10, 2023

OverOrion May 11, 2023

MrAnno May 10, 2023

OverOrion May 11, 2023

MrAnno May 10, 2023

OverOrion May 11, 2023

Potential null pointer fixes / checks #4132

Are you sure you want to change the base?

Potential null pointer fixes / checks #4132

Conversation

OverOrion commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

kira-syslogng commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OverOrion May 7, 2023 • edited

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

bazsi left a comment

Choose a reason for hiding this comment

MrAnno commented Oct 1, 2022

MrAnno commented May 7, 2023

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

OverOrion May 7, 2023 •

edited