Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prevent panic caused by nil session recorder #10792

Merged
merged 5 commits into from
Mar 4, 2022
Merged

Prevent panic caused by nil session recorder #10792

merged 5 commits into from
Mar 4, 2022

Conversation

rosstimothy
Copy link
Contributor

In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder which
becomes a non-nil events.StreamWriter. So when the session tries
to close the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.

The same path in startExec handled this correctly, by assigning
the return value to the recorder only after checking the error. This
moves the common logic to create a session recorder used by startExec
and startInteractive into newRecorder. Both functions correctly check
the return value from newRecorder and only assign the session recorder
in the event the returned err == nil.

teleport[15368]: panic: runtime error: invalid memory address or nil pointer dereference
teleport[15368]: [signal SIGSEGV: segmentation violation code=0x1 addr=0x100 pc=0x112968e]
teleport[15368]: goroutine 3890290 [running]:
teleport[15368]: github.com/gravitational/teleport/lib/events.(*AuditWriter).Close(0x0, {0x4, 0x4299365})
teleport[15368]:         /go/src/github.com/gravitational/teleport/lib/events/auditwriter.go:377 +0x2e
teleport[15368]: github.com/gravitational/teleport/lib/srv.(*session).Close.func1.1()
teleport[15368]:         /go/src/github.com/gravitational/teleport/lib/srv/sess.go:637 +0xbe
teleport[15368]: created by github.com/gravitational/teleport/lib/srv.(*session).Close.func1
teleport[15368]:         /go/src/github.com/gravitational/teleport/lib/srv/sess.go:631 +0x73
systemd[1]: teleport.service: Main process exited, code=exited, status=2/INVALIDARGUMENT
systemd[1]: teleport.service: Failed with result 'exit-code'.

@rosstimothy
Prevent panic caused by nil session recorder
317b9d0 
In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder as
a non-nil events.StreamWriter. So when the session tries to close
the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.
@rosstimothy rosstimothy force-pushed the tross/fix_audit_writer_panic branch from 28d7b4c to 317b9d0 Compare March 3, 2022 15:38
smallinsky
smallinsky approved these changes Mar 4, 2022
View reviewed changes
lib/srv/sess_test.go Outdated Show resolved Hide resolved
@rosstimothy rosstimothy merged commit d03087c into master Mar 4, 2022
@rosstimothy rosstimothy deleted the tross/fix_audit_writer_panic branch March 4, 2022 22:00
rosstimothy added a commit that referenced this pull request Mar 4, 2022
@rosstimothy
Prevent panic caused by nil session recorder (#10792)
575a05d 
* Prevent panic caused by nil session recorder

In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder as
a non-nil events.StreamWriter. So when the session tries to close
the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.
@rosstimothy rosstimothy mentioned this pull request Mar 4, 2022
Merged
rosstimothy added a commit that referenced this pull request Mar 4, 2022
@rosstimothy
Prevent panic caused by nil session recorder (#10792)
e7197a5 
* Prevent panic caused by nil session recorder

In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder as
a non-nil events.StreamWriter. So when the session tries to close
the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.
@rosstimothy rosstimothy mentioned this pull request Mar 4, 2022
Merged
rosstimothy added a commit that referenced this pull request Mar 4, 2022
@rosstimothy
Prevent panic caused by nil session recorder (#10792)
d1fdecc 
* Prevent panic caused by nil session recorder

In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder as
a non-nil events.StreamWriter. So when the session tries to close
the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.
@rosstimothy rosstimothy mentioned this pull request Mar 4, 2022
Merged
rosstimothy added a commit that referenced this pull request Mar 7, 2022
@rosstimothy
Prevent panic caused by nil session recorder (#10792) (#10875)
9b68bdc 
* Prevent panic caused by nil session recorder

In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder as
a non-nil events.StreamWriter. So when the session tries to close
the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.
rosstimothy added a commit that referenced this pull request Mar 8, 2022
@rosstimothy
Prevent panic caused by nil session recorder (#10792)
c8ca543 
* Prevent panic caused by nil session recorder

In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder as
a non-nil events.StreamWriter. So when the session tries to close
the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.
rosstimothy added a commit that referenced this pull request Mar 10, 2022
@rosstimothy
Prevent panic caused by nil session recorder (#10792) (#10874)
504dc74 
In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder as
a non-nil events.StreamWriter. So when the session tries to close
the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.
rosstimothy added a commit that referenced this pull request Mar 10, 2022
@rosstimothy
Prevent panic caused by nil session recorder (#10792) (#10876)
7771cc2 
* Prevent panic caused by nil session recorder

In startInteractive the session recorder was being assigned the
return value of events.NewAuditWriter, even if it returned an
error. This causes problems because the nil *events.AuditWriter
that is returned in this case ends up being stored in recorder as
a non-nil events.StreamWriter. So when the session tries to close
the check on recorder != nil is mistakenly true and recorder.Close
is called on a nil *events.AuditWriter - which results in a panic.
@webvictim webvictim mentioned this pull request Apr 19, 2022
Closed
@webvictim webvictim mentioned this pull request Jun 8, 2022
Closed
@alexatcanva alexatcanva mentioned this pull request Oct 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smallinsky smallinsky smallinsky approved these changes

@gabrielcorado gabrielcorado gabrielcorado approved these changes

Assignees
No one assigned
Labels
backport-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rosstimothy @gabrielcorado @smallinsky