Skip to content

chore: prevent nil derefs in non-critical paths #11411

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

Merged
merged 3 commits into from
Jan 4, 2024
Merged

chore: prevent nil derefs in non-critical paths #11411

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3613ec
chore: prevent nil derefs in non-critical paths
Emyrk Jan 4, 2024
ae7c34c
log line when nil time detected
Emyrk Jan 4, 2024
55258d0
fix
mafredri Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions cli/cliui/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,12 @@ func Agent(ctx context.Context, writer io.Writer, agentID uuid.UUID, opts AgentO

switch agent.LifecycleState {
case codersdk.WorkspaceAgentLifecycleReady:
sw.Complete(stage, agent.ReadyAt.Sub(*agent.StartedAt))
sw.Complete(stage, safeDuration(sw, agent.ReadyAt, agent.StartedAt))
case codersdk.WorkspaceAgentLifecycleStartTimeout:
sw.Fail(stage, 0)
sw.Log(time.Time{}, codersdk.LogLevelWarn, "Warning: A startup script timed out and your workspace may be incomplete.")
case codersdk.WorkspaceAgentLifecycleStartError:
sw.Fail(stage, agent.ReadyAt.Sub(*agent.StartedAt))
sw.Fail(stage, safeDuration(sw, agent.ReadyAt, agent.StartedAt))
// Use zero time (omitted) to separate these from the startup logs.
sw.Log(time.Time{}, codersdk.LogLevelWarn, "Warning: A startup script exited with an error and your workspace may be incomplete.")
sw.Log(time.Time{}, codersdk.LogLevelWarn, troubleshootingMessage(agent, "https://coder.com/docs/v2/latest/templates#startup-script-exited-with-an-error"))
Expand All @@ -221,7 +221,7 @@ func Agent(ctx context.Context, writer io.Writer, agentID uuid.UUID, opts AgentO
case agent.LifecycleState.ShuttingDown():
// We no longer know if the startup script failed or not,
// but we need to tell the user something.
sw.Complete(stage, agent.ReadyAt.Sub(*agent.StartedAt))
sw.Complete(stage, safeDuration(sw, agent.ReadyAt, agent.StartedAt))
return errAgentShuttingDown
}
}
Expand All @@ -238,13 +238,13 @@ func Agent(ctx context.Context, writer io.Writer, agentID uuid.UUID, opts AgentO
sw.Log(time.Now(), codersdk.LogLevelWarn, "Wait for it to reconnect or restart your workspace.")
sw.Log(time.Now(), codersdk.LogLevelWarn, troubleshootingMessage(agent, "https://coder.com/docs/v2/latest/templates#agent-connection-issues"))

disconnectedAt := *agent.DisconnectedAt
disconnectedAt := agent.DisconnectedAt
for agent.Status == codersdk.WorkspaceAgentDisconnected {
if agent, err = fetch(); err != nil {
return xerrors.Errorf("fetch: %w", err)
}
}
sw.Complete(stage, agent.LastConnectedAt.Sub(disconnectedAt))
sw.Complete(stage, safeDuration(sw, agent.LastConnectedAt, disconnectedAt))
}
}
}
Expand All @@ -257,6 +257,25 @@ func troubleshootingMessage(agent codersdk.WorkspaceAgent, url string) string {
return m
}

// safeDuration returns a-b. If a or b is nil, it returns 0.
// This is because we often dereference a time pointer, which can
// cause a panic. These dereferences are used to calculate durations,
// which are not critical, and therefor should not break things
// when it fails.
// A panic has been observed in a test.
func safeDuration(sw *stageWriter, a, b *time.Time) time.Duration {
if a == nil || b == nil {
if sw != nil {
Copy link
Member

Choose a reason for hiding this comment

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

No need for sw to be an optional param here, IMO 🤔 (i.e. no need to check).

Copy link
Member Author

Choose a reason for hiding this comment

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

I just really didn't want to accidently introduce a new place to deref a nil lol

// Ideally the message includes which fields are <nil>, but you can
// use the surrounding log lines to figure that out. And passing more
// params makes this unwieldy.
sw.Log(time.Now(), codersdk.LogLevelWarn, "Warning: Failed to calculate duration from a time being <nil>.")
}
return 0
}
return a.Sub(*b)
}

type closeFunc func() error

func (c closeFunc) Close() error {
Expand Down