Potential null pointer fixes / checks by OverOrion · Pull Request #4132 · syslog-ng/syslog-ng

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

      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

  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

                {
-                  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

  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 <[email protected]>

Signed-off-by: Szilárd Parrag <[email protected]>

Refactored this function to have it decied whether the old buffer should be freed or not. Signed-off-by: Szilárd Parrag <[email protected]>

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


-      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

 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 <[email protected]>

Signed-off-by: Szilard Parrag <[email protected]>

Signed-off-by: Szilárd Parrag <[email protected]>

bazsi reviewed Sep 13, 2022

View reviewed changes

bazsi suggested 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 <[email protected]>

lib: make sure template is function is successfully retrieved

1c1cd95

Signed-off-by: Szilárd Parrag <[email protected]>

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 <[email protected]>

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 suggested 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 <[email protected]>

affile: add affile_dw_unset_owner

ce7b018

Signed-off-by: Szilard Parrag <[email protected]>

lib: fix possible null pointer dereference in logreader

1107829

Signed-off-by: Szilárd Parrag <[email protected]>

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

HofiOne added the abandoned label Jun 7, 2024

MrAnno mentioned this pull request Jul 19, 2024

Null-pointer fixes axoflow/axosyslog#216

Merged

OverOrion closed this Jul 19, 2024

HofiOne mentioned this pull request Jul 22, 2024

Further null ptr fixes #5035

Merged

Conversation

OverOrion commented Sep 13, 2022

Uh oh!

github-actions bot commented Sep 13, 2022

Uh oh!

kira-syslogng commented Sep 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OverOrion May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bazsi left a comment

Choose a reason for hiding this comment

Uh oh!

MrAnno commented Oct 1, 2022

Uh oh!

MrAnno commented May 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

OverOrion May 7, 2023 •

edited

Loading