mrusme / neonmodem

Neon Modem Overdrive
https://neonmodem.com
GNU General Public License v3.0
582 stars 22 forks source link

Fix segfault when entering incorrect password #33

Closed greenfoo closed 1 year ago

greenfoo commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thank you very much!