================================================================================ SKYCHART (Cartes du Ciel) - BUG AND BAD-PRACTICE REPORT ================================================================================ Scope of review --------------- 105 .pas units, ~128,700 lines, extracted from skychart-master.zip, plus a second pass over skychart-master_with_forms.zip which added 63 .lfm form files and the .lpi/.lpr project files. The 105 .pas files are byte-identical between the two archives, so every finding below applies to both. No compiler was available (and neither were the LCL, BGRABitmap, Synapse or SynEdit dependencies), so this is a static review: the source was scanned with pattern analysis and the hits were then read and verified by hand. Line numbers refer to the files as supplied. Findings A1-A13 and B1-B10 come from the Pascal sources alone and did not depend on the form files. A14 and the build-configuration notes in A3 and B5 come from the .lfm/.lpi files in the second archive. Two findings - A5 and A12 - were reached by reasoning about the code rather than by observing the program, and should be confirmed against actual runtime behaviour before any change is made. Findings are grouped by severity. Each entry gives the location, what the code does, why it is wrong, and a suggested fix. Companion file: skychart-improvements.txt (refactoring and design suggestions). ================================================================================ SECTION A - CONFIRMED LOGIC BUGS ================================================================================ -------------------------------------------------------------------------------- A1. Wrong variable used in the planet bitmap cache check File: cu_plot.pas, line 2207 (TSplot.GetBodyImage) Severity: HIGH (wrong rendering, wasted work) -------------------------------------------------------------------------------- idx := FCacheBMP.Search(IntToStr(ipla)); if idx >= 0 then begin NewScan := (abs(FCacheBMP.GetJD(idx) - jdt) > 0.000693) or (FCacheBMP.GetDiameter(idx) <> ds) or (abs(FCacheBMP.GetProt(idx) - pa) > 0.2) or ((idx = C_Jupiter) and (FCacheBMP.GetGRS(idx) <> gw)); <-- here `idx` is the position of the entry inside the TFPHashList cache. The body being drawn is `ipla`. C_Jupiter is the planet number 5 (u_orbits.pas:45), not a list index. The two are unrelated: the cache key is IntToStr(ipla) and the index depends purely on insertion and eviction order. Consequences: - Jupiter's cached image is not invalidated when the Great Red Spot longitude (gw) changes, so the GRS is drawn at a stale longitude until some other criterion (date, diameter, rotation) forces a refresh. - Whatever unrelated body happens to occupy cache slot 5 is compared against Jupiter's GRS value and gets needlessly re-rendered (an xplanet run, which is expensive). Fix: ((ipla = C_Jupiter) and (FCacheBMP.GetGRS(idx) <> gw)); -------------------------------------------------------------------------------- A2. UCAC4 star designation is built with a 0-based string assumption File: u_290.pas, lines 1072-1074 (readdatabase290) Severity: HIGH (wrong identifiers shown to the user) -------------------------------------------------------------------------------- str(nr_star+1000000:7,naamst); {add zeros by 1000000 and later remove 1} naam2:=name_regio+'-'+naamst[1]+naamst[2]+naamst[3]+naamst[4]+naamst[5] +naamst[6]; {naamst[0]contains the "1" and skip this one} The trick is: add 1,000,000 to get leading zeros, then drop the leading '1'. The comment says naamst[0] holds the '1', which is C thinking. In Pascal both AnsiString and ShortString are 1-based (for ShortString, index 0 is the length byte). So naamst[1] IS the leading '1', and the code keeps it while dropping the last digit. Example: nr_star = 286833 -> naamst = '1286833' -> output '128683'. Correct output would be '286833'. `naam2` is copied straight into `rec.star.id` in cu_catalog.pas:6382, so every UCAC4 designation reported from the .290 databases (mouse-click identification, text search) is wrong. Fix: naam2 := name_regio + '-' + copy(naamst, 2, 6); (Using Copy also removes six string concatenations per star.) -------------------------------------------------------------------------------- A3. Record size read from the file header is never validated File: u_290.pas, lines 942-954 and 1065 Severity: HIGH (buffer overflow / division by zero / silent corruption) -------------------------------------------------------------------------------- reader_stars.read(database2,110); {read 110-byte header} if database2[109]=' ' then record_size:=11 else record_size:=ord(database2[109]); {5,6,7,9,10 or 11 bytes record} nr_records:= trunc((thefile_stars.size-110)/record_size); ... reader_stars.read(buf2,record_size); `buf2` is declared as `array[1..11] of byte` (u_290.pas:822). Three distinct problems: (a) DIVISION BY ZERO. If byte 109 of the header is #0 (truncated, zero-filled or corrupted file), record_size becomes 0 and the trunc(...) expression divides by zero. (b) BUFFER OVERFLOW. If byte 109 is anything above 11 - say 0x20, which is a plausible value in a mis-identified file - `read(buf2, record_size)` writes that many bytes into an 11-byte global buffer. buf2 sits in the unit's global data next to `thefile_stars`, `Reader_stars` and the p5..p11 pointers, so the overflow corrupts live object references. This is reachable from any .290 file the user drops into the catalog directory. (c) UNHANDLED SIZES. The `case record_size of` at line 954 has arms for 5, 6, 9, 10 and 11 only. There is no arm for 7 - even though the comment on line 944 lists 7 as legal - and there is no `else`. For an unhandled size the case falls through silently, ra2/dec2/mag2 keep the values from the previous iteration, and the loop continues emitting duplicated phantom stars until maxmag or EOF stops it. Note on compiler checks: range checking does NOT mitigate (b) in any build mode. `reader_stars.read(buf2, record_size)` is a raw memory write through a pointer, not an indexed array access, so {$R+} offers no protection even in the build modes that enable it (see B5 for the per-mode table; the `release` and `windows` modes have it off entirely). The validation has to be explicit. Fix: validate before use, and reject the file otherwise. if not (record_size in [5,6,7,9,10,11]) then begin closedatabase; readdatabase290 := false; exit; end; and add an `else` arm to the case that also aborts. Ideally also size buf2 from a named constant and assert record_size <= SizeOf(buf2). -------------------------------------------------------------------------------- A4. Unguarded arccos in the asteroid ephemeris File: cu_planet.pas, lines 2721-2722 (TPlanet.Asteroid) Severity: MEDIUM-HIGH (floating point exception / crash) -------------------------------------------------------------------------------- elong := arccos((rr * rr + dist * dist - r * r) / (2.0 * rr * dist)); phase := arccos((r * r + dist * dist - rr * rr) / (2.0 * r * dist)); The near-identical comet routine 130 lines earlier does guard itself (cu_planet.pas:2587-2595): n1 := (rr * rr + dist * dist - r * r) / (2.0 * rr * dist); if abs(n1) <= 1 then elong := arccos(n1) else elong := 0; FPC implements arccos(x) as arctan2(sqrt(1-x*x), x). For |x| marginally above 1 - which happens routinely from rounding in the law-of-cosines form, especially for objects close to the Sun or to the observer - sqrt() of a negative value raises an invalid-operation floating point exception. There is also no check that `dist` and `r` are non-zero before dividing. Fix: apply the same clamp used in TPlanet.Comet, and guard the denominators. Better still, factor the clamped computation out into one shared helper used by both routines (see improvements file, item I3). -------------------------------------------------------------------------------- A5. AngularDistance returns "maximum distance" for near-coincident points File: u_projection.pas, lines 990-1013 Severity: MEDIUM (objects at the search centre can be missed) -------------------------------------------------------------------------------- if (ar1 = ar2) and (de1 = de2) then Result := 0.0 else begin ... c3 := (s1 * s2) + (c1 * c2 * cos((ar1 - ar2))); if abs(c3) <= 1 then Result := double(arccos(c3)) else Result := pi2; <-- wrong fallback end; ... except Result := pi2; <-- wrong fallback end; Two issues: (a) The exact-equality fast path only catches bit-identical coordinates. Two positions that differ by 1e-12 rad produce c3 = 1.0000000000000002, which fails `abs(c3) <= 1` and returns pi2 (2*pi) - the largest possible value - when the true separation is essentially zero. (b) pi2 is not even a legal angular distance. The maximum separation on a sphere is pi. Returning 2*pi as an error sentinel is confusing and, because callers compare against a radius, silently means "infinitely far". Affected call sites: cu_catalog.pas:4913 if AngularDistance(...) < radius then -> an object sitting exactly on the search centre can be excluded from its own search result. pu_info.pas:223 rdist[i].r := AngularDistance(...) -> the nearest object is sorted to the far end of the "nearby objects" list. cu_skychart.pas:8896 -> a target exactly at the chart centre is judged to be off-chart. Fix: clamp instead of bailing out. if c3 > 1 then c3 := 1 else if c3 < -1 then c3 := -1; Result := arccos(c3); and change the except fallback to pi (or, better, let the exception propagate - with the clamp in place there is nothing left to throw). -------------------------------------------------------------------------------- A6. Comet error recovery permanently corrupts the orbital elements File: cu_planet.pas, lines 2547-2560 (TPlanet.OrbRect) Severity: MEDIUM -------------------------------------------------------------------------------- if comelem.oe < 1 then try eliptique; except if (comelem.oe>0.9999) then comelem.oe:=1 // retry with parabolic orbit else raise; end; if comelem.oe = 1 then parabolique; if comelem.oe > 1 then hyperbolique; When the Kepler solver fails to converge for a near-parabolic comet, the handler writes back into the object's stored element set. That value is not restored afterwards, so: - The eccentricity reported in the object-info panel and in any exported element list becomes exactly 1.0 instead of the real value. - Every subsequent ephemeris for that comet takes the parabolic branch, even at epochs where the elliptic solver would have converged fine. - The failure is invisible: no log entry, no user notification. Also note the `if comelem.oe = 1` test is an exact floating point comparison on a value parsed from a text file; an element set with e = 0.99999999999 falls through all three branches in the (unlikely but possible) case where the elliptic branch was skipped. Fix: use a local variable for the dispatch. ecc := comelem.oe; if ecc < 1 then try eliptique except if ecc > 0.9999 then ecc := 1 else raise; end; if SameValue(ecc, 1.0, 1e-12) then parabolique else if ecc > 1 then hyperbolique; and log the fallback so it can be diagnosed. -------------------------------------------------------------------------------- A7. Use-after-free and data races in the TCP server File: cu_tcpserver.pas (lines 127-129, 178-215, 246-320) and pu_main.pas (lines 11209-11211, 11974-11983, 12003-12010) Severity: HIGH (intermittent crashes, hard to reproduce) -------------------------------------------------------------------------------- Several related defects: (a) Fsock is freed but not nilled. TTCPThrd.Execute, finally block (cu_tcpserver.pas:314-318): Fsock.SendString(msgBye + crlf); Fsock.CloseSocket; Fsock.Free; cmd.Free; TTCPThrd.Senddata, called from the MAIN thread, then does: if Fsock <> nil then ... After Free, Fsock is a dangling pointer, not nil, so the guard passes and the main thread writes through freed memory. (b) The thread array is never cleared. Threads are created with FreeOnTerminate := True, and TTCPDaemon.ThrdTerminate (line 127) does only: ThrdActive[i] := False; TCPThrd[i] is left pointing at an object that is about to destroy itself. pu_main.pas:11209 dereferences it: (TCPDaemon.ThrdActive[i]) and (TCPDaemon.TCPThrd[i] <> nil) and (TCPDaemon.TCPThrd[i].sock <> nil) ... Short-circuit evaluation saves the common case, but ThrdActive[i] is a plain Boolean written from the worker thread and read from the main thread with no synchronisation and no memory barrier. Between the read of ThrdActive[i] and the dereference of TCPThrd[i] the thread can terminate and free itself. (c) The termination callback runs on the worker thread. cu_tcpserver.pas:312-313: if assigned(FTerminate) then FTerminate(id); Every other cross-thread call in this unit uses Synchronize(). This one does not, yet it mutates state the main thread reads. (d) The whole of Execute is wrapped in `try ... except end` (line 319-320), so any of the above failures is swallowed and the connection just dies. (e) `cmd` is created in the constructor but freed in Execute. If Start fails or the thread is destroyed without running, cmd leaks. There is no destructor. Fix: - FreeAndNil(Fsock) and FreeAndNil(cmd), and move the cleanup into an overridden Destroy. - Have ThrdTerminate clear the slot: TCPThrd[i] := nil (and call it through Synchronize, or protect the array with a TCriticalSection). - Do not use FreeOnTerminate for objects the main thread keeps references to; either keep ownership in the daemon and Free them explicitly, or hand the main thread an id rather than a pointer. -------------------------------------------------------------------------------- A8. Memory leak: TStringList never freed in cmd_GetScopeRates File: fu_chart.pas, lines 5528-5550 Severity: MEDIUM (unbounded leak on a scriptable command) -------------------------------------------------------------------------------- srate := TStringList.Create; Fpop_scope.GetScopeRates(n0, srate); if n0 > 0 then ... else Result := msgFailed; end; end; <-- no Free, no try/finally srate is a local variable and is never freed on any path. This command is reachable from the TCP/script interface, so a client polling scope rates leaks a TStringList per call. Fix: srate := TStringList.Create; try ... finally srate.Free; end; -------------------------------------------------------------------------------- A9. Memory leak on the cancel path of the script editor File: pu_scriptengine.pas, lines 2524-2548 Severity: LOW-MEDIUM -------------------------------------------------------------------------------- if (node.Data <> nil) and (TObject(node.Data) is TStringList) then s := (TStringList(node.Data)) else s := TStringList.Create; <-- new list, owner not yet assigned ... Fpascaleditor.ShowModal; if Fpascaleditor.ModalResult = mrOk then begin s.Assign(...); node.Data := s; <-- ownership transferred only here CompileAndSave; end; If the user cancels the dialog and node.Data was nil, the freshly created list is orphaned. Every cancelled edit of a new Button/Combo/Event script leaks. Fix: track whether the list was created locally and free it in an else branch, or attach it to node.Data immediately after creation. -------------------------------------------------------------------------------- A10. Destructors that do not call inherited Destroy Files: u_CacheBMP.pas:87-90 (TCacheBMP.Destroy) u_orbits.pas:988-995 (TOrbits.Destroy) Severity: LOW now, LATENT HIGH -------------------------------------------------------------------------------- destructor TCacheBMP.Destroy; begin Self.Clear; FList.Free; end; <-- no `inherited Destroy;` Both classes currently descend directly from TObject, whose Destroy is empty, so nothing leaks today. But the destructors are declared `override`, which means the contract is "chain to the parent". The moment either class gains a real ancestor - or is changed to descend from TComponent, TPersistent or an interfaced object - the parent cleanup silently stops running, and the bug will be very hard to trace back to here. Fix: append `inherited Destroy;` to both. (FPC's -vh will warn about this if hints are enabled.) -------------------------------------------------------------------------------- A11. Comment character tested against the untrimmed line File: cu_skychart.pas, lines 9234-9236 and 9257-9258 (Tskychart.LoadHorizon) Severity: LOW-MEDIUM -------------------------------------------------------------------------------- repeat readln(f, buf) until EOF(f) or ((trim(buf) <> '') and (buf[1] <> '#')); if (trim(buf) = '') or (buf[1] = '#') then exit; The blank test uses trim(buf) but the comment test uses buf[1], the raw first character. A horizon file with an indented comment such as # my local horizon passes both tests (trim is non-empty, buf[1] is a space) and is handed to StrToFloat, which raises. The exception is caught by the bare `except end` at line 9299, so the horizon silently ends up flat and the user gets no diagnostic at all. Fix: trim once into a local and test that. line := trim(buf); ... until EOF(f) or ((line <> '') and (line[1] <> '#')); -------------------------------------------------------------------------------- A12. Horizon wrap-around cells are not reset File: cu_skychart.pas, lines 9220-9221 and 9295-9296 Severity: LOW (visible glitch at azimuth 0/360) -------------------------------------------------------------------------------- for i := 1 to 360 do cfgsc.horizonlist[i] := 0; <-- indices 0 and 361 skipped horizonlist is `array [0..361] of single` (u_constant.pas:915). Elements 0 and 361 are the wrap-around guard cells used by the projection code; they are only written inside the `finally` block at lines 9295-9296, which is reached only when `fileexists(fname)` is true. So when the user switches from a valid horizon file to a missing or unreadable one, the array is cleared to zero except for slots 0 and 361, which keep the previous file's values. The rendered horizon has a spike at due north. Fix: `for i := 0 to 361 do cfgsc.horizonlist[i] := 0;` -------------------------------------------------------------------------------- A13. striphtml does not recognise tags with attributes or self-closing tags File: u_util.pas, lines 828-876 Severity: LOW (cosmetic, affects object-detail text) -------------------------------------------------------------------------------- '>': begin intag := False; if tag = 'p' then Result := Result + crlf; if tag = 'br' then Result := Result + crlf; end; `tag` accumulates everything between '<' and '>', so it equals 'p' only for a bare `

`. Real-world markup such as `

`, `

`, `
` or `
` produces tag values of 'p class="note"', 'P', 'br/' and 'br ' - none of which match, so no line break is emitted and the detail text runs together. There is also no handling of '<' appearing inside an attribute value. Fix: normalise before comparing. tagname := LowerCase(trim(words(tag, '', 1, 1))); tagname := StringReplace(tagname, '/', '', [rfReplaceAll]); if (tagname = 'p') or (tagname = 'br') then Result := Result + crlf; -------------------------------------------------------------------------------- A14. Published component fields that have no object in the .lfm Files: fu_config_chart.pas:228 Panel1: TPanel fu_config_system.pas:81 Language: TTabSheet Severity: LOW (latent trap) Source: cross-check of the 63 .lfm files against the form class declarations in the matching .pas units -------------------------------------------------------------------------------- Both fields are declared in the published section of their form class, where the LCL streaming system would normally bind them to a design-time component. Neither name appears in the corresponding .lfm, and neither is assigned a `TSomething.Create` anywhere in the unit. They are therefore nil for the entire life of the form. fu_config_chart.pas:228 Panel1: TPanel fu_config_chart.lfm contains no object named Panel1. Referenced nowhere else in the unit. fu_config_system.pas:81 Language: TTabSheet fu_config_system.lfm declares its tab sheets as Page1..Page5, TabSheet1/2 and TabSheet5/6. There is no `Language` tab; this looks like a leftover from a rename. The six other case-insensitive matches for "language" in this unit are `cmain.language`, an unrelated identifier. Nothing touches either field today, so there is no current fault. The risk is future: the declarations look like working components, so the next person to add `Panel1.Visible := False` or `Language.TabVisible := False` gets a silent access violation with no obvious cause. Lazarus does not warn about published fields with no matching streamed object. Fix: delete both declarations. Checks that came back clean on the same cross-reference, for the record: - OnXxx handlers referenced in .lfm with no matching method in the form class (would make the form fail to load): 0 occurrences - duplicate component names within a form: 0 occurrences - duplicate keyboard shortcuts within a form: 0 occurrences - TTimer design-time state: all 25 instances across the project are Enabled = False, and no code assumes otherwise. ================================================================================ SECTION B - SYSTEMIC BAD PRACTICE ================================================================================ -------------------------------------------------------------------------------- B1. 77 empty exception handlers Severity: HIGH (this is the single biggest maintainability problem) -------------------------------------------------------------------------------- The pattern except end; occurs 77 times. It turns every failure inside the protected block - including programming errors such as access violations and range errors - into silence. Worst concentrations: cu_database.pas 15 occurrences, wrapping entire catalogue load/delete routines (LoadCometFile, DelComet, LoadAstExt, LoadAstFam, LoadCountryList, LoadWorldLocation, LoadUSLocation, ...). A failed import reports success. pu_main.pas 16 occurrences, several around configuration save: SaveDefault (8932), SaveChartConfig (9423), SaveQuickSearch (9749, 9774, 9798). A failed config write is indistinguish- able from a successful one; the user loses settings with no warning. cu_ascommount.pas / cu_ascomrestmount.pas 6 each, some of which are legitimate (probing optional ASCOM interface members) but are indistinguishable from the accidental ones. cu_skychart.pas:9299, cu_fits.pas:1505 and :1707, cu_plot.pas:681, u_constant.pas:2999 (inside Tconf_main.Destroy - a swallowed destructor failure leaks everything downstream). Note also u_util.pas:2829 which reads `except;` - an empty handler with a stray semicolon. Recommendation: - Where the exception genuinely is expected (optional ASCOM property, missing file), catch the specific class and add a one-line comment saying why. - Everywhere else, at minimum call WriteTrace(E.Message) - the project already has a tracing facility - and set an error result the caller can see. - Never wrap a Save* routine in a silent handler. -------------------------------------------------------------------------------- B2. Mutable global state disguised as constants File: u_290.pas, lines 38 and 812 Severity: MEDIUM -------------------------------------------------------------------------------- const maxmag : integer=999; const record_size:integer=11; These are writable typed constants (legal because the unit is `{$mode delphi}`, where $J defaults to on) and both are assigned at runtime. Combined with the unit-level `buf2`, `area290`, `thefile_stars`, `Reader_stars` and `dec9_storage`, the entire .290 reader is a single global state machine: - It is not reentrant. Two charts drawing simultaneously would corrupt each other's read position. - It cannot be moved to a background thread, which is exactly what a star catalogue reader should be. - `readdatabase290` depends on `reset290index` having been called first, with nothing enforcing it. Recommendation: wrap the reader in a class holding its own file handle, buffer and cursor. The public API barely changes and the globals disappear. -------------------------------------------------------------------------------- B3. Reading an out parameter before it is written File: u_290.pas, lines 868 and 884-886 Severity: LOW (works today, fragile) -------------------------------------------------------------------------------- function readdatabase290(...; out ra2,dec2, mag2, Bp_Rp : double): boolean; ... if ( (file_open=0) or (nr_records<=0) or ((searchmode<>'T') and (mag2>maxmag)) ) then `mag2` is an `out` parameter. FPC does not initialise out parameters of non-managed types, so on the first call mag2 holds whatever was on the stack. The code is only correct because short-circuit evaluation ({$B-}, the default) never reaches the third operand on the first call, since file_open = 0. Compile the unit with {$B+} - or reorder those conditions during a future edit - and the function starts reading uninitialised memory. Fix: declare the parameter `var`, or assign mag2 := 0 at the top of the function. -------------------------------------------------------------------------------- B4. No-op statements and dead code Severity: LOW -------------------------------------------------------------------------------- u_projection.pas:1357 h := h; u_projection.pas:1408 h := h; Both sit in the `0:` arm of a refraction method case ("no refraction for testing"). Harmless, but they produce compiler hints that train developers to ignore hints. An empty `begin end` with the comment is clearer. Also present throughout: large blocks of commented-out code (for example cu_tcpserver.pas:239-242, cu_plot.pas:2247, fu_chart.pas and pu_main.pas in several places) and `{ TODO : ... }` markers such as pu_main.pas:2792. These belong in version control history, not in the source. -------------------------------------------------------------------------------- B5. Unchecked I/O, and runtime checks disabled in the shipping build mode Severity: MEDIUM (revised upward after reading cdc.lpi) -------------------------------------------------------------------------------- `{$I-}` (I/O checking off) is enabled in seven units - cu_catalog.pas, cu_skychart.pas, pu_calendar.pas, u_290.pas, u_satellite.pas (twice), u_unzip.pas and u_util.pas (twice) - but IOResult is never read afterwards in any of them. Turning off automatic error raising without checking the result code means I/O failures are simply invisible. The project file cdc.lpi shows this is not confined to those seven units. Per build mode: build mode Range Overflow I/O Stack ShowHints ---------- ----- -------- --- ----- --------- release off off off off (unset) windows off off off off False debug on on on on False debug heap on on on on False win64 on on on on False cocoa on on on on False gtk2 on on on on False qt6 on on on on False So in the `release` and `windows` modes there is no I/O checking, no range checking, no overflow checking and no stack checking anywhere in the program. The source-level {$I-} directives override the project setting locally in the other modes, so those seven units have I/O checking off regardless of how the program is built. Two knock-on points: - The per-mode inconsistency is itself a hazard: `release` and `windows` have a different runtime-check profile from the five platform modes (win64/cocoa/gtk2/qt6/debug) that are presumably what actually ships. A bug that the platform modes would trap becomes silent corruption in `release`. Whichever profile is correct, it should be the same in all of them. - `ShowHints` is False in every mode that sets it. The hints that FPC emits for a destructor with no `inherited` call (bug A10) and for a self- assignment such as `h := h;` (B4) are therefore suppressed project-wide. See improvements item I7. Additionally, `closefile(f)` appears in `finally` blocks (for example cu_skychart.pas:9284) where `reset(f)` may have failed, which raises inside the cleanup handler. Recommendation: drop {$I-}, or check IOResult immediately after every guarded operation. Consider replacing the textfile API with TStreamReader/TStringList, which report failures as exceptions. -------------------------------------------------------------------------------- B6. Inconsistent floating point comparison of Julian dates Severity: LOW -------------------------------------------------------------------------------- cu_plot.pas:2204 (abs(FCacheBMP.GetJD(idx) - jdt) > 0.000693) <- tolerance cu_plot.pas:2425 (FCacheBMP.GetJD(idx) <> dt) <- exact The same cache, the same field, two different comparison strategies within 220 lines. The exact comparison works only as long as both sides originate from the identical double; any recomputation makes the cache miss every time. Similar exact comparisons on floats: cu_plot.pas:2436 and 2473 (`flatten <> 1`), cu_planet.pas:2559-2561 (`comelem.oe = 1`), cu_catalog.pas:6738 (`distfact<>1.0`). Recommendation: use Math.SameValue with an explicit epsilon, and pick one tolerance per quantity. -------------------------------------------------------------------------------- B7. FreeOnTerminate set before the inherited constructor File: cu_tcpserver.pas, lines 246-249 Severity: LOW -------------------------------------------------------------------------------- constructor TTCPThrd.Create(Hsock: TSocket); begin FreeOnTerminate := True; inherited Create(True); ... This happens to work because instance memory is zeroed before the constructor body runs and TThread.Create does not touch FFreeOnTerminate. It relies on RTL implementation detail that has changed between FPC versions in the past. Fix: call `inherited Create(True);` first. -------------------------------------------------------------------------------- B8. Cross-thread GUI access without Synchronize Severity: MEDIUM -------------------------------------------------------------------------------- Most of cu_tcpserver.pas is careful (Synchronize(ShowError), Synchronize (ExecuteCmd), Synchronize(GetActiveChart)), which makes the exceptions stand out: cu_tcpserver.pas:312-313 FTerminate(id) called directly from Execute. cu_tcpserver.pas:207-212 TCPThrd[n].senddata(...) called from the daemon thread while the worker thread may be using Fsock. cu_tcpserver.pas:196-200 busy-wait `while (TCPThrd[n].Fsock = nil) ... sleep (100)` - reading a field written by another thread with no barrier, for up to 10 seconds. The comment at cu_tcpserver.pas:277 ("for debugging only, not thread safe!") shows the author is aware of the hazard, which makes the remaining cases worth auditing. -------------------------------------------------------------------------------- B9. Bounds checking absent on cache accessors File: u_CacheBMP.pas, lines 165-190 Severity: LOW -------------------------------------------------------------------------------- function TCacheBMP.GetBMP(Index: integer): TBGRABitmap; begin Result := PCacheBMP_Data(FList[Index]).BMP end; No range check and no nil check. Callers currently guard with `if idx >= 0`, but nothing in the class enforces it, and Delete() renumbers the remaining entries - so an index captured before a Delete silently refers to a different object. The class would be considerably safer if lookups were done by ID rather than by index. -------------------------------------------------------------------------------- B10. Global format settings mutated at startup Files: pu_main.pas:2739-2742, pu_tray.pas:426-429 Severity: LOW (design tradeoff, worth documenting) -------------------------------------------------------------------------------- DefaultFormatSettings.DecimalSeparator := '.'; DefaultFormatSettings.ThousandSeparator := ','; DefaultFormatSettings.DateSeparator := '/'; DefaultFormatSettings.TimeSeparator := ':'; Application.UpdateFormatSettings := False; This is the pragmatic way to make the ~90 bare StrToFloat / FloatToStr calls scattered through the code behave consistently when parsing catalogue and configuration files. The cost is that every number shown to the user is formatted in US convention regardless of locale, which is a visible regression for a translated application - and any library unit that parses a float now silently depends on a global having been set by the main form. Recommendation: keep a module-level `CatalogFormat: TFormatSettings` with '.' as the decimal separator, pass it explicitly to StrToFloat/FloatToStr for file and protocol I/O, and leave DefaultFormatSettings alone for UI text. ================================================================================ SUMMARY ================================================================================ Confirmed logic bugs 14 (A1-A14) of which likely user-visible today 6 (A1,A2,A5, A7,A8,A12) of which crash or memory-safety risks 4 (A3,A4,A7, A8) of which latent only (no current fault) 2 (A10,A14) Systemic bad practices 10 (B1-B10) Of these, A1-A13 and B1-B10 were found in the Pascal sources; A14 and the build-configuration material in A3 and B5 required the .lfm and .lpi files. Highest-value fixes, in order: 1. A3 - validate record_size in u_290.pas (memory safety, one if-statement) 2. A7 - nil the freed socket/thread pointers in cu_tcpserver.pas 3. A1 - ipla vs idx in cu_plot.pas:2207 (one character) 4. A2 - UCAC4 designation off-by-one in u_290.pas:1074 5. A5 - clamp the arccos argument in AngularDistance 6. B1 - replace the 77 empty except blocks with logged handlers 7. B5 - align the runtime-check settings across all eight build modes, and turn ShowHints back on None of these require restructuring. A1, A2, A4, A5, A8, A10, A12 and A14 are each a one- to three-line change; B5 is a project-options edit.