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.

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

fix(#13844): canonicalize job to avoid nil pointer deference #13845

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

tristanpemble
Copy link
Contributor

@tristanpemble tristanpemble commented Jul 19, 2022

I am new to this codebase, so unsure I have the best approach, but this fixes my issue in #13844. I traced the issue down to the nil pointer here, which is called from this path.

This code is called after the job registration succeeds on the server. I believe the root of the issue is that when submitting an API job via the CLI, it is canonicalized server-side, but not client-side. Registration succeeds on the server, and then this code path is called. It references paths that have not been canonicalized with their default values, and fails.

Another option would be to canonicalize the job before registration (maybe in the JobGetter when loading the job?), but being new to the codebase, I am wary of the unintended change in behavior that might create for endusers.

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 19, 2022

CLA assistant check
All committers have signed the CLA.

@Amier3
Copy link
Contributor

Amier3 commented Jul 20, 2022

Hey @tristanpemble

Thanks for making this PR and congrats on the first contribution. We'll take a look at this soon 😄

@Amier3 Amier3 added this to Needs Triage in Nomad - Community Issues Triage via automation Jul 20, 2022
@tgross tgross self-requested a review July 25, 2022 13:27
@tgross tgross self-assigned this Jul 25, 2022
@tgross tgross moved this from Needs Triage to Triaging in Nomad - Community Issues Triage Jul 25, 2022
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @tristanpemble! Thanks for this PR!

The fix you've got here will totally work for the case of the CLI, but it'll still be broken for other consumers of the HTTP API. The panic you've hit in #13844 is at api/jobs.go#L788. The p pointer is nil, and that causes the panic when we try to access one of its fields. But in this case, we have a safe fallback behavior later in the Next method. So a fix like the diff below should fix both the CLI panic you've hit and prevent anyone else from hitting the same bug if they don't call Canonicalize():

-if *p.SpecType == PeriodicSpecCron {
+if p != nil && *p.SpecType == PeriodicSpecCron {

If you can make this change I'd be happy to get this PR merged. You can also add a changelog entry file at .changelog/13845.txt with the following text:

```improvement
api: Fixed a bug where accessing the next periodic invocation for a job could panic if not set
```

@@ -345,6 +345,9 @@ func (c *JobRunCommand) Run(args []string) int {

evalID := resp.EvalID

// #13844: canonicalize the job in case it was a partial API definition
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, there's no need to have the issue number in the comments. That's what git blame is for 😀

@tgross
Copy link
Member

tgross commented Feb 1, 2023

Hi! 👋 I want to get this PR landed in the upcoming Nomad 1.5.0, so I've pushed a commit with the requested fix and a changelog entry. Once that's green I'll get this merged.

@tgross tgross added backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line backport/1.4.x backport to 1.4.x release line type/bug and removed stage/waiting-reply labels Feb 1, 2023
@tgross tgross added this to the 1.5.0 milestone Feb 1, 2023
@tgross tgross merged commit 67f8f22 into hashicorp:main Feb 1, 2023
Nomad - Community Issues Triage automation moved this from In Progress to Done Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2.x backport to 1.1.x release line backport/1.3.x backport to 1.3.x release line backport/1.4.x backport to 1.4.x release line type/bug
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants