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 segfault when entering incorrect password #33

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

greenfoo
Copy link
Contributor

When configuring an account with an incorrect password system.New() returns nil which causes a segmentation fault when accessing sys:

sys, err := system.New(...)
if err != nil {
    ...
} else {
    ...
}
c.AddSystem(&sys) <--------- SIGSEGV here

One option would be to panic() when err != nil, but this goes against the structure of the already existing code, which iterates over all configured systems, collects error messages from each of them (if any), and later (when returning from loadSystems()), prints all these errors before panic()'ing.

Thus, we want not to panic() here, but still we want the nil pointer not to be accessed. The solution is simple: just include the code that accesses the sys object in the else branch:

sys, err := system.New(...)
if err != nil {
    ...
} else {
    ...
    c.AddSystem(&sys)
}

Note that, as explained before, when entering a wrong password the binary will still fail (wich a panic()) but at least now each of the reassons why each system could not be initialized will be printed to the log file.

@tedwardd
Copy link
Contributor

That seems like a really easy fix for the bulk of this issue, thank you!

Something I've been playing around with in #29 is building a custom error type for password errors. Maybe this is something we can expand on that here and instead of just generically panicking we can return a password error and handle it more gracefully... Just some food for thought. I'm not even sure I like my approach yet, but in my experience it's usually preferred to pass errors back up the stack to the root method and handle them there vs. panicking, unless something unexpected has happened.

@mrusme mrusme added bug Something isn't working enhancement New feature or request labels Jun 21, 2023
@mrusme mrusme merged commit f0ac5c9 into mrusme:master Jun 21, 2023
@mrusme
Copy link
Owner

mrusme commented Jun 21, 2023

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants