diff --git a/src/tui.zig b/src/tui.zig index bd31af6..1ea8339 100644 --- a/src/tui.zig +++ b/src/tui.zig @@ -4,7 +4,6 @@ const zfin = @import("root.zig"); const fmt = @import("format.zig"); const Money = @import("Money.zig"); const cli = @import("commands/common.zig"); -const portfolio_loader = @import("portfolio_loader.zig"); const stderr = @import("stderr.zig"); const keybinds = @import("tui/keybinds.zig"); const tab_framework = @import("tui/tab_framework.zig"); @@ -378,19 +377,10 @@ pub const SymbolData = struct { pub fn clear(self: *SymbolData, allocator: std.mem.Allocator) void { if (self.candles) |c| allocator.free(c); if (self.dividends) |d| zfin.Dividend.freeSlice(allocator, d); - if (self.etf_profile) |profile| { - if (profile.holdings) |h| { - for (h) |holding| { - if (holding.symbol) |s| allocator.free(s); - allocator.free(holding.name); - } - allocator.free(h); - } - if (profile.sectors) |s| { - for (s) |sec| allocator.free(sec.name); - allocator.free(s); - } - } + // Use EtfProfile.deinit so we free every owned field — + // including `symbol` and `name`, which an earlier + // hand-rolled inline cleanup here was silently leaking. + if (self.etf_profile) |profile| profile.deinit(allocator); self.* = .{}; } diff --git a/src/tui/analysis_tab.zig b/src/tui/analysis_tab.zig index 109032f..dbdfcff 100644 --- a/src/tui/analysis_tab.zig +++ b/src/tui/analysis_tab.zig @@ -121,8 +121,16 @@ pub const tab = struct { /// PortfolioData and are reset by pd's own load path — /// nothing to free here for those. /// - /// Eagerly recomputes only if this tab is currently active — - /// otherwise next `activate` will lazy-load. + /// Deliberately does NOT eager-rebuild, even when this tab + /// is active. The reload broadcast fires BEFORE pd.reload + /// resets the portfolio arena; rebuilding now would store + /// borrowed pointers (e.g. BreakdownItem.label borrowing + /// from ClassificationEntry.bucket) that get freed seconds + /// later when the arena resets, producing use-after-free + /// crashes on the next draw. The orchestrator + /// (portfolio_tab.reloadPortfolioFile) calls + /// `app.loadTabData()` AFTER pd.reload completes, which + /// re-activates the active tab against fresh data. pub fn onPortfolioReload(state: *State, app: *App) void { if (state.result) |*ar| ar.deinit(app.allocator); state.result = null; @@ -130,9 +138,6 @@ pub const tab = struct { // reload's broadcast already invalidated it via pd's own // reset path. Nothing to free here. state.loaded = false; - if (app.active_tab == .analysis) { - tab.activate(state, app) catch |err| std.log.debug("analysis activate failed: {t}", .{err}); - } } }; @@ -509,6 +514,29 @@ test "tab.init produces zero-defaulted state" { try testing.expectEqual(zfin.analysis.Granularity.mid, state.sector_granularity); } +test "onPortfolioReload clears state without eager rebuild" { + // Regression for the use-after-free crash where eager-rebuild + // inside onPortfolioReload stored borrowed pointers from the + // about-to-be-freed portfolio arena. Contract: this hook + // MUST clear state and set loaded=false, but MUST NOT call + // activate() or otherwise rebuild from app.portfolio (which + // is about to be reset by pd.reload). + // + // We can't easily verify "no eager rebuild" mechanically + // without a full App harness, but we CAN verify the + // post-conditions are exactly state-cleared. + var state: State = .{ + .loaded = true, + // result stays null — non-null would need allocator to free. + }; + var dummy_app: tui.App = undefined; // not touched when result is null + + tab.onPortfolioReload(&state, &dummy_app); + + try testing.expectEqual(false, state.loaded); + try testing.expect(state.result == null); +} + test "handleAction cycles sector granularity coarse → mid → fine → coarse" { var state: State = .{}; var dummy_app: tui.App = undefined; // handleAction doesn't touch app diff --git a/src/tui/history_tab.zig b/src/tui/history_tab.zig index ffcf29e..18dcfdf 100644 --- a/src/tui/history_tab.zig +++ b/src/tui/history_tab.zig @@ -215,6 +215,33 @@ pub const tab = struct { loadData(state, app); } + /// Drop cached timeline and compare view on portfolio reload. + /// `state.tl` borrows symbol strings from the previous + /// portfolio's memory, and `compare_resources.then_live_map` + /// / `now_live_map` borrow keys from `app.portfolio` — all + /// of which `pd.reload` is about to free. Drop them now. + /// + /// Deliberately does NOT eager-rebuild, even when this tab + /// is active. The reload broadcast fires BEFORE pd.reload + /// resets the portfolio arena; rebuilding now would store + /// borrowed pointers that get freed seconds later. The + /// orchestrator (portfolio_tab.reloadPortfolioFile) calls + /// `app.loadTabData()` AFTER pd.reload completes, which + /// re-activates the active tab against fresh data. + /// + /// Selections + cursor are reset because the row indices + /// they refer to may no longer be valid after the new + /// timeline is loaded (different snapshot count, etc.). + pub fn onPortfolioReload(state: *State, app: *App) void { + freeLoaded(state, app); + state.loaded = false; + state.cursor = 0; + state.selections = .{ null, null }; + state.table_first_line = 0; + state.table_row_count = 0; + state.expanded_buckets.clearRetainingCapacity(); + } + pub const tick = framework.noopTick(State); pub fn handleAction(state: *State, app: *App, action: Action) void { diff --git a/src/tui/performance_tab.zig b/src/tui/performance_tab.zig index d1638b3..cdc4997 100644 --- a/src/tui/performance_tab.zig +++ b/src/tui/performance_tab.zig @@ -157,7 +157,22 @@ fn loadData(state: *State, app: *App) void { app.symbol_data.etf_loaded = true; if (app.svc.getEtfProfile(app.symbol, .{})) |etf_result| { if (etf_result.data.isEtf()) { + // Take ownership of the EtfProfile data. We + // deliberately don't call etf_result.deinit + // here — the data fields (symbol, name, + // holdings, sectors) are now owned by + // symbol_data and will be freed by + // symbol_data.clear() on next symbol change. + // The FetchResult wrapper itself is a struct + // with no separate heap allocation; dropping + // it leaks nothing. app.symbol_data.etf_profile = etf_result.data; + } else { + // Non-ETF: the profile data was still allocated + // by getEtfProfile (symbol dupe at minimum, + // possibly name from Wikidata fallback). Free + // it via the FetchResult contract. + etf_result.deinit(); } } else |_| {} } diff --git a/src/tui/portfolio_tab.zig b/src/tui/portfolio_tab.zig index 153d90d..f0c7cf4 100644 --- a/src/tui/portfolio_tab.zig +++ b/src/tui/portfolio_tab.zig @@ -1755,6 +1755,15 @@ pub fn buildWelcomeScreenLines( /// Goes through the same `loadPortfolioFromPaths` the initial /// load uses, so a manual reload sees the merged view of every /// `portfolio*.srf` in the resolved directory — same as the CLI. +/// +/// Lifecycle: broadcast onPortfolioReload BEFORE pd.reload so +/// every tab clears derived state that borrows from the +/// portfolio arena. Tabs MUST NOT eager-rebuild from those +/// hooks — pd.reload is about to free the arena out from under +/// them. Instead, after pd.reload completes, app.loadTabData() +/// re-activates the currently-active tab against fresh data. +/// Inactive tabs stay in `loaded = false` and lazy-rebuild on +/// next switch. pub fn reloadPortfolioFile(state: *State, app: *App) void { if (app.portfolio.paths.len == 0) { app.setStatus("No portfolio file to reload"); @@ -1766,6 +1775,7 @@ pub fn reloadPortfolioFile(state: *State, app: *App) void { // models, cursor / expansion indices that pointed into the // about-to-be-freed portfolio data). MUST happen BEFORE // pd.reload starts freeing/recreating the underlying data. + // Tabs MUST NOT eager-rebuild from this hook (see above). app.broadcast("onPortfolioReload", .{}); // Reload watchlist file too (if separate). pd doesn't read @@ -1794,14 +1804,24 @@ pub fn reloadPortfolioFile(state: *State, app: *App) void { }; if (app.portfolio.summary == null) return; - // Activate this tab's UI rebuild now that the data is - // ready. (onPortfolioReload above already cleared the old - // UI state; this re-derives the new state.) + // Always rebuild portfolio_tab UI state (regardless of + // which tab is active) — the onPortfolioReload broadcast + // cleared it above, and if we leave it empty the user + // could switch back without an activate firing in some + // paths. Cheap (sort + filter on ~30 holdings). sortPortfolioAllocations(state, app); buildAccountList(state, app); recomputeFilteredPositions(state, app); rebuildPortfolioRows(state, app); + // Re-activate the currently-active tab (whatever it is) so + // it rebuilds derived state from the freshly-reloaded + // portfolio data. Tabs that gate on `state.loaded` already + // got `loaded = false` from the broadcast above, so this + // call triggers their full re-load. For portfolio_tab + // itself this is idempotent with the manual rebuild above. + app.loadTabData(); + app.setStatus("Portfolio reloaded from disk"); } diff --git a/src/tui/projections_tab.zig b/src/tui/projections_tab.zig index 99c4f41..8d18b5e 100644 --- a/src/tui/projections_tab.zig +++ b/src/tui/projections_tab.zig @@ -416,15 +416,18 @@ pub const tab = struct { /// portfolio's memory; invalidate before the underlying data /// is freed. /// - /// Eagerly recomputes only if this tab is active — otherwise - /// the next `activate` will lazy-load. + /// Deliberately does NOT eager-rebuild, even when this tab + /// is active. The reload broadcast fires BEFORE pd.reload + /// resets the portfolio arena; rebuilding now would store + /// borrowed pointers that get freed seconds later when the + /// arena resets, producing use-after-free crashes on the + /// next draw. The orchestrator + /// (portfolio_tab.reloadPortfolioFile) calls + /// `app.loadTabData()` AFTER pd.reload completes, which + /// re-activates the active tab against fresh data. pub fn onPortfolioReload(state: *State, app: *App) void { - if (app.active_tab == .projections) { - tab.reload(state, app) catch |err| std.log.debug("projections reload failed: {t}", .{err}); - } else { - freeLoaded(state, app); - state.loaded = false; - } + freeLoaded(state, app); + state.loaded = false; } }; diff --git a/src/tui/review_tab.zig b/src/tui/review_tab.zig index ad009d1..bda989d 100644 --- a/src/tui/review_tab.zig +++ b/src/tui/review_tab.zig @@ -338,8 +338,16 @@ pub const tab = struct { /// edited `acknowledgments.srf` while the TUI was running. /// (Same rationale as the user-requested `reload` action.) /// - /// Eagerly recomputes only when this tab is active — - /// otherwise next `activate` lazy-loads. + /// Deliberately does NOT eager-rebuild, even when this tab + /// is active. The reload broadcast fires BEFORE pd.reload + /// resets the portfolio arena; rebuilding now would store + /// borrowed pointers (e.g. ReviewRow.bucket borrowing from + /// ClassificationEntry.bucket) that get freed seconds later + /// when the arena resets, producing use-after-free crashes + /// or visual corruption on the next draw. The orchestrator + /// (portfolio_tab.reloadPortfolioFile) calls + /// `app.loadTabData()` AFTER pd.reload completes, which + /// re-activates the active tab against fresh data. pub fn onPortfolioReload(state: *State, app: *App) void { if (state.view) |*v| v.deinit(app.allocator); state.view = null; @@ -350,9 +358,6 @@ pub const tab = struct { state.loaded = false; state.cursor = 0; state.expanded_finding = null; - if (app.active_tab == .review) { - tab.activate(state, app) catch |err| std.log.debug("review activate failed: {t}", .{err}); - } } /// Mouse handling: left-click on the column-header row sorts