New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent freeing a possible invalid pointer from journald #31231

Merged
merged 1 commit into from Feb 22, 2017

Conversation

Projects
None yet
10 participants
@mlaventure
Contributor

mlaventure commented Feb 21, 2017

Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

--

May fix #30433

After looking at the code, at the moment the only explanation I can muster is that somehow sd_journal_get_cursor() failed and left the cursor variable with an invalid value.

@boutros @amatsus any chance you'd be able to test this patch?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Feb 21, 2017

Member

SGTM

Member

tonistiigi commented Feb 21, 2017

SGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 22, 2017

Member

ping @nalind PTAL

Member

thaJeztah commented Feb 22, 2017

ping @nalind PTAL

@boutros

This comment has been minimized.

Show comment
Hide comment
@boutros

boutros Feb 22, 2017

@boutros @amatsus any chance you'd be able to test this patch?

Unforunatley I don't know a way to trigger the crash - it just happend at irregular intervals, days to sometimes hours. It happened in to different, but equally specced production servers. We downgraded to 1.12.6 for now.

Thanks for giving this attention @mlaventure :)

boutros commented Feb 22, 2017

@boutros @amatsus any chance you'd be able to test this patch?

Unforunatley I don't know a way to trigger the crash - it just happend at irregular intervals, days to sometimes hours. It happened in to different, but equally specced production servers. We downgraded to 1.12.6 for now.

Thanks for giving this attention @mlaventure :)

@amatsus

This comment has been minimized.

Show comment
Hide comment
@amatsus

amatsus Feb 22, 2017

Contributor

I tested this patch with docker 1.13.1. I get same results.
docker-crash-report.txt

I also downgraded to 1.12.6 for now.

Contributor

amatsus commented Feb 22, 2017

I tested this patch with docker 1.13.1. I get same results.
docker-crash-report.txt

I also downgraded to 1.12.6 for now.

@nalind

This comment has been minimized.

Show comment
Hide comment
@nalind

nalind Feb 22, 2017

Contributor

@thaJeztah Looks like a good correctness fix to me.

Contributor

nalind commented Feb 22, 2017

@thaJeztah Looks like a good correctness fix to me.

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Feb 22, 2017

Member

apparently the issue in #30433 is still happening though #31231 (comment) :(

Member

runcom commented Feb 22, 2017

apparently the issue in #30433 is still happening though #31231 (comment) :(

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Feb 22, 2017

Contributor

@amatsus thanks for testing!

@tonistiigi re-reading the crash, something is weird. The stacktrace implies that the free from drainJournal is called when the go routine exits, which would mean after sd_journal_close() has been called.

I'll update the patch to only close the journal in readLogs see if that helps.

Contributor

mlaventure commented Feb 22, 2017

@amatsus thanks for testing!

@tonistiigi re-reading the crash, something is weird. The stacktrace implies that the free from drainJournal is called when the go routine exits, which would mean after sd_journal_close() has been called.

I'll update the patch to only close the journal in readLogs see if that helps.

Prevent freeing a possible invalid pointer from journald
Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Feb 22, 2017

Contributor

@amatsus do you mind testing that updated version? I still haven't found a way to reproduce this.

Contributor

mlaventure commented Feb 22, 2017

@amatsus do you mind testing that updated version? I still haven't found a way to reproduce this.

@amatsus

This comment has been minimized.

Show comment
Hide comment
@amatsus

amatsus Feb 22, 2017

Contributor

@mlaventure My bad, I used the f5007f2 patch. I use the 81630df patch, so I get a good result. Many thanks!

From f5007f2e8de6dc55fada9298ccd2bd34854935bb Mon Sep 17 00:00:00 2001
From: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
Date: Tue, 21 Feb 2017 11:07:57 -0800
Subject: [PATCH] Prevent freeing a possible invalid pointer from journald

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
---
 daemon/logger/journald/read.go | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/daemon/logger/journald/read.go b/daemon/logger/journald/read.go
index 541fab70..afb7b41 100644
--- a/daemon/logger/journald/read.go
+++ b/daemon/logger/journald/read.go
@@ -237,7 +237,10 @@ drain:

        // free(NULL) is safe
        C.free(unsafe.Pointer(oldCursor))
-       C.sd_journal_get_cursor(j, &cursor)
+       if C.sd_journal_get_cursor(j, &cursor) != 0 {
+               // ensure that we won't be freeing an address that's invalid
+               cursor = nil
+       }
        return cursor
 }
Contributor

amatsus commented Feb 22, 2017

@mlaventure My bad, I used the f5007f2 patch. I use the 81630df patch, so I get a good result. Many thanks!

From f5007f2e8de6dc55fada9298ccd2bd34854935bb Mon Sep 17 00:00:00 2001
From: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
Date: Tue, 21 Feb 2017 11:07:57 -0800
Subject: [PATCH] Prevent freeing a possible invalid pointer from journald

Signed-off-by: Kenfe-Mickael Laventure <mickael.laventure@gmail.com>
---
 daemon/logger/journald/read.go | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/daemon/logger/journald/read.go b/daemon/logger/journald/read.go
index 541fab70..afb7b41 100644
--- a/daemon/logger/journald/read.go
+++ b/daemon/logger/journald/read.go
@@ -237,7 +237,10 @@ drain:

        // free(NULL) is safe
        C.free(unsafe.Pointer(oldCursor))
-       C.sd_journal_get_cursor(j, &cursor)
+       if C.sd_journal_get_cursor(j, &cursor) != 0 {
+               // ensure that we won't be freeing an address that's invalid
+               cursor = nil
+       }
        return cursor
 }
@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Feb 22, 2017

Contributor

@amatsus Not at all your fault, I updated the patch after your last comment :).

Thanks for testing it, we'll wait a bit to see if it comes back for you since you seems to be easily affected

Contributor

mlaventure commented Feb 22, 2017

@amatsus Not at all your fault, I updated the patch after your last comment :).

Thanks for testing it, we'll wait a bit to see if it comes back for you since you seems to be easily affected

@nalind

This comment has been minimized.

Show comment
Hide comment
@nalind

nalind Feb 22, 2017

Contributor

(@mlaventure's re-reading of the crash also made me take a closer look at the sequence of events in followJournal, which prompted #31263.)

Contributor

nalind commented Feb 22, 2017

(@mlaventure's re-reading of the crash also made me take a closer look at the sequence of events in followJournal, which prompted #31263.)

@thaJeztah thaJeztah added this to PRs in 17.03.2-maybe Feb 22, 2017

@@ -237,7 +237,10 @@ drain:
// free(NULL) is safe
C.free(unsafe.Pointer(oldCursor))
C.sd_journal_get_cursor(j, &cursor)
if C.sd_journal_get_cursor(j, &cursor) != 0 {
// ensure that we won't be freeing an address that's invalid

This comment has been minimized.

@tiborvass

tiborvass Feb 22, 2017

Collaborator

I must be slow, but i dont understand this. How does this prevent freeing an invalid address?

@tiborvass

tiborvass Feb 22, 2017

Collaborator

I must be slow, but i dont understand this. How does this prevent freeing an invalid address?

This comment has been minimized.

@nalind

nalind Feb 23, 2017

Contributor

@tiborvass the man page for sd_journal_get_cursor() doesn't mention what happens to the cursor pointer when the function returns a failure code. The implementation ends up calling asprintf(), which explicitly says the pointer's content is unspecified, so after a failure code we have to take a step to make sure that the pointer's not still uninitialized, and that it hasn't been set to some garbage value.

@nalind

nalind Feb 23, 2017

Contributor

@tiborvass the man page for sd_journal_get_cursor() doesn't mention what happens to the cursor pointer when the function returns a failure code. The implementation ends up calling asprintf(), which explicitly says the pointer's content is unspecified, so after a failure code we have to take a step to make sure that the pointer's not still uninitialized, and that it hasn't been set to some garbage value.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Feb 22, 2017

Collaborator

LGTM

Collaborator

tiborvass commented Feb 22, 2017

LGTM

@vieux vieux merged commit f67eb69 into moby:master Feb 22, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31026 has succeeded
Details
janky Jenkins build Docker-PRs 39638 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10703 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.04.0 milestone Feb 22, 2017

@vieux vieux referenced this pull request Feb 22, 2017

Merged

17.03 cherry-picks #31266

@vieux vieux modified the milestones: 17.03.0, 17.04.0 Feb 22, 2017

@thaJeztah thaJeztah removed this from PRs in 17.03.2-maybe Feb 23, 2017

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