Skip to content

logging: do not read more that the buf size#361

Merged
haircommander merged 2 commits into
containers:mainfrom
giuseppe:do-not-read-after-buffer
Nov 3, 2022
Merged

logging: do not read more that the buf size#361
haircommander merged 2 commits into
containers:mainfrom
giuseppe:do-not-read-after-buffer

Conversation

@giuseppe
Copy link
Copy Markdown
Member

@giuseppe giuseppe commented Nov 3, 2022

msg_len doesn't take into account NUL bytes that could be present in
the buffer, while g_strdup_printf("MESSAGE=%s%s", partial_buf, buf)
does and would stop writing the string once it finds a NUL byte. This
would lead to read after the buffer end.

Build the message string manually so that the calculated length
reflects the buffer size.

Closes: #315

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

sd_journal_sendv returns a negative errno, so negate it before usage.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Copy link
Copy Markdown
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@haircommander
Copy link
Copy Markdown
Collaborator

@giuseppe one lint issue https://github.com/containers/conmon/pull/361/checks?check_run_id=9269929279 otherwise LGTM, thanks for taking this on

msg_len doesn't take into account NUL bytes that could be present in
the buffer, while g_strdup_printf("MESSAGE=%s%s", partial_buf, buf)
does and would stop writing the string once it finds a NUL byte.  This
would lead to read after the buffer end.

Build the message string manually so that the calculated length
reflects the buffer size.

Closes: containers#315

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the do-not-read-after-buffer branch from f234c45 to 955586b Compare November 3, 2022 13:51
@evverx
Copy link
Copy Markdown

evverx commented Nov 3, 2022

Thanks! I can't reproduce that issue anymore. I also wrote a (half-baked) fuzz target and it hasn't been able to find anything else either so far. Unfortunately OSS-Fuzz makes it extremely hard to add fuzz targets with external dependencies like libsystemd and glib so I'm not sure how to bring it there at this point without having to maintain glib and systemd builds. I have to run a lot fuzz targets elsewhere because of that as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

segmentation fault with podman and ocrmypdf

4 participants