# GoStations Issues **Project:** gostations (console radio station browser/player) **Analysis date:** 2026-06-05 **Scope:** Focused review of obvious bugs, crash paths, error handling failures, and high-impact correctness issues. Performed via source inspection, test runs (`go test`), live execution, and forced error-path reproduction (bad DNS, connection failures, config edge cases). This document captures findings from the current source tree. The existing `build/gostations` binary is stale (different build paths in debug info) — always rebuild from current sources for testing. --- ## Critical Bugs (Panics and Hard Crashes) ### 1. Nil pointer dereference on `http.Get` failure (most common network error path) **Location:** `browser.go:50` (GetStations) ```go resp, err := http.Get(urlstr) if err != nil { log.Print(err.Error()) } defer func(Body io.ReadCloser) { err := Body.Close() ... }(resp.Body) // <--- resp is nil on most errors ... err = json.NewDecoder(resp.Body).Decode(&data) ``` **Impact:** Any transient or permanent failure contacting the radio-browser API (DNS, connect refused, timeout, etc.) causes an immediate panic instead of graceful degradation. **Reproduction:** - Set `radio_browser.api=localhost` (or any host that resolves but refuses HTTPS on 443) in a temp `radiostations.ini` under `XDG_CONFIG_HOME`. - Run `gostations -c "Gambia"`. - Observed: `panic: runtime error: invalid memory address or nil pointer dereference` at the `defer` evaluation line. **Related:** No `http.Client` with timeout/context is used anywhere. Searches can hang indefinitely. ### 2. Panic in API host selection on DNS failure or empty result **Location:** `browser.go:25-38` (RandomIP, nslookup, reverseLookup, GetApiHost) ```go func RandomIP(iplist []net.IP) net.IP { rand.NewSource(time.Now().Unix()) // result discarded! randomIndex := rand.Intn(len(iplist)) // panics if len==0 ... } func nslookup(hostname string) net.IP { iprecords, _ := net.LookupIP(hostname) // error ignored return RandomIP(iprecords) } func reverseLookup(ipAddr net.IP) string { ptr, _ := net.LookupAddr(ipAddr.String()) return ptr[0] // panics if len==0 } ``` **Impact:** `GetApiHost()` is called on **every** `StationSearch` (before the actual station query). Failure here kills the entire program with either: - `panic: invalid argument to Intn` - index-out-of-range panic **Reproduction:** Set `radio_browser.api=nonexistent.invalid.host.example` → full panic stack through `StationSearch` → `main`. **Additional problems:** - The `NewSource` call is dead code (return value thrown away; does not affect the global RNG used by `Intn`). In Go 1.14-era this made "random" API host selection deterministic. Modern autoseeding hides the bug but the intent is broken. - No caching of the chosen API host (DNS + reverse lookup repeated for every search). - `GetStations` is called with the result of `GetApiHost()` with zero defensive coding. ### 3. `subExecute` always returns an error on success + produces garbage output **Location:** `commander.go:28-38` ```go func subExecute(program string, args ...string) ([]byte, error) { cmd := exec.Command(program, args...) cmd.Stdin = os.Stdin cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err := cmd.Run() if err != nil { fmt.Printf("%v\n", err) } return cmd.CombinedOutput() // !!! } ``` **Impact:** - `CombinedOutput()` sees `Stdout` already set → returns `nil, errors.New("exec: Stdout already set")`. - Callers ignore the error: `stdout, _ := subExecute(...)`; `fmt.Println(stdout)`. - After every successful play (mpv exits cleanly on 'q'), the user sees a stray `[]` line, then the menu reappears. - This is the origin of the `[]` visible in the README examples after "Exiting... (Quit)". **Test failure:** `TestSubExecute_Live` (under `-short`) fails with exactly this error. **Usage site:** `radiomenu.go:32` (inside the wmenu action callback, after the blocking play). ### 4. `createIniFile` continues writing after failures (fragile, relies on runtime luck) **Location:** `filer.go:10-46` - Records `MkdirAll` / `Create` errors but proceeds unconditionally to `file.Write`. - On failure, `file` is `nil` (documented `os` behavior). - Relies on `*os.File` methods returning errors instead of panicking when called on `nil` (observed behavior in current Go; not guaranteed or clean). - Defer for `Close` is registered even on failure paths. - Multiple "invalid argument" errors get appended for every subsequent write. **Impact:** Auto-generation of `radiostations.ini` on first run (or missing file) is not robust. `configStat` then does `log.Fatal` on the first error in the list (with a typo: "Erorr"). **Test note:** The happy-path tests pass because they exercise success cases only. Error-collection tests would expose the mess. --- ## Error Handling, Correctness, and Robustness Issues ### 5. Widespread ignored errors + lossy error paths - `stations.go:61`: `stations, _ := StationSearch(...)` — search errors are silently dropped. Main proceeds with potentially `nil` slice. - `browser.go:94-102` (StationSearch): On any `GetStations` error the data is discarded and `nil` is returned. On `notok` path, `err` is returned even when it is `nil`. - `browser.go:62-66` (GetStations): `err = json...` overwrites any prior `http.Get` error variable; partial data + last `err` is returned in some cases. - `config.go:75,80,85,90`: All the convenience wrappers (`api()`, `player()`, `options()`, `maxitems()`) discard `Config` errors. - Lookups in `nslookup`/`reverseLookup` discard errors. - `radiomenu.go:35`: `log.Fatal` inside a menu action callback. ### 6. Misleading / broken config error message **Location:** `config.go:69` ```go if optval == "" { return "", errors.New("no value for option '%s'") // %s is never formatted } ``` Always returns the literal string containing `'%s'`. Reproduced via temp ini missing expected keys. ### 7. Command injection risk (Unix `isInstalled`) **Location:** `commander.go:19` ```go cmd = exec.Command("/bin/sh", "-c", "command -v "+name) ``` `name` comes from the user-editable `player.command` value in `radiostations.ini`. A malicious or accidentally malformed value executes arbitrary shell commands during the precheck. (`subExecute` itself uses `exec.Command(program, ...)` directly and is safer for the player step, but the check is not.) ### 8. Inverted test logic for "live" tests + missing coverage - `*_Live` tests in `*_test.go` files only run when `-short` is passed (the `if !testing.Short() { t.Skip }` guard is backwards). - `browser_test.go` is essentially empty. - No tests exercise the panic paths in `GetStations`, `RandomIP`, `nslookup`, config missing-option, `createIniFile` failure, etc. - `TestPrecheck_Live` etc. rely on real global state (player present in PATH, config file creation). ### 9. Other correctness / quality issues - `stations.go:33-40`: Custom `flag.Usage` + `flag.PrintDefaults()` produces slightly mangled help (tabs immediately after bool flags like `-vShow...`). - `main` has a zero-arg special case for usage, but `-h` is handled by the `flag` package (works by accident). No explicit help flag handling. - `radiomenu.go:34`: Recursive `menu.Run()` inside the selection action after playback (potential for deep stacks on repeated listen/return cycles; state of the wmenu instance is reused in a non-obvious way). - `config.go:52`: `configparser.Delimiter = "="` is mutated globally on every `Config()` call. - `filer.go` + `config.go`: Manual string concatenation for paths (`+ "/gostations/" + ...`) instead of `filepath.Join`. `configStat` also hard-codes the `gostations` subdirectory name. - `str2int` (config.go:14) returns 9999 on any `Atoi` error (including overflow on 32-bit platforms). Test expects `4294967296` which only works on 64-bit `int`. - `GetStations` URL: `?...&limit=...` when `params.Encode() == ""` produces a leading `?&`. - No version information is embedded at runtime except via fragile `-ldflags "-X main.version=..."` (the `var version string` at package level). - Heavy use of `log.Print`/`log.Fatal` mixes diagnostic output with user-facing behavior and aborts without cleanup. --- ## Non-Bug Recommendations / Improvement Opportunities - **Caching & performance:** Resolve the radio-browser API host once and reuse it (or fall back to a static list of known endpoints). The current DNS dance is expensive and repeated. - **HTTP client:** Use a single `*http.Client` with reasonable `Timeout` and context support. - **Player execution:** Rewrite or replace `subExecute`. For long-running players the current "live + capture" approach is fundamentally conflicted. Consider just letting the player inherit stdio and not trying to capture/return anything after it exits. - **Menu restart:** After playback, either restart the menu cleanly (new `RadioMenu` call) or use wmenu's built-in looping/return mechanisms instead of recursion + `os.Exit(0)` from inside an action. - **Config handling:** Perform validation once at startup (as noted in the existing README TODO). Cache the parsed config. Use `fmt.Errorf` with proper verbs. Provide a `--config` flag or at least clear diagnostics when the ini is invalid/missing keys. - **Error strategy:** Return errors up to `main`, print a user-friendly message, and exit with non-zero status. Avoid `log.Fatal` for normal control flow. - **Randomness:** Fix `RandomIP` to actually use a per-call `*rand.Rand` if the goal is to pick among A records. - **Testing:** Add table-driven tests (or httptest) for the error paths in browser.go. Fix the `-short` guard logic or rename the tests. Add a build tag or separate integration suite. - **Modernization:** - Stop using vendoring (or at least stop committing the entire `vendor/` tree for a small CLI). - Update ancient dependencies (testify v1.4.0, configparser from 2019, etc.). - Use `//go:embed` or `debug.ReadBuildInfo` for version instead of ldflags + global. - Use `os.UserConfigDir()` + `filepath.Join` properly (with fallback for the old XDG logic if desired). - **Security / robustness:** Validate/sanitize the player command (or at least document that the ini must be trusted). Add a `--player` override flag. - **UX:** Truncation at 40 chars is aggressive on modern terminals. Consider making the menu nicer (the existing TODO mentions color/dashboard). - **Existing README TODO items** (still relevant): - "Change the precheck, to do ini file validation once, rather than every time a config value is called for." - "Add a way to capture favorite selections." - "Add color or a dashboard to the menu and player." --- ## How to Reproduce Key Issues Quickly 1. **DNS / host selection panic:** ```sh mkdir -p /tmp/badgost/gostations cat > /tmp/badgost/gostations/radiostations.ini <