From b40b90d7af062d067b0c486cc89babd0674dffea Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Sun, 6 Sep 2026 19:06:08 +0200 Subject: [PATCH] fix: a library comes from the system directory, and the catalogue from one place Two findings of the outside security review. One of them turned out to be wrong about the fix and right about the risk, and the guard written for it found a second call the review never looked at. S7 asked for syscall.NewLazySystemDLL instead of NewLazyDLL in the free space lookup. THAT FUNCTION DOES NOT EXIST: measured, the compiler calls it undefined, and syscall has no LoadLibraryEx and no LOAD_LIBRARY_SEARCH_ constants either. It lives in golang.org/x/sys/windows, which untouchable rule 11 keeps out of the command line binary - the comment beside that call has said so since 2026-08-25. And that call is safe as written: kernel32.dll is a KnownDLL, measured in the registry, so it is already mapped and the loader never consults a search order for it. The review was right about the next call rather than that one, and the guard written to hold that found it: internal/gui loaded uxtheme.dll by name, and uxtheme.dll is NOT a KnownDLL - the same registry key, thirty seven entries, and it is not among them. The Windows search order puts the directory the program was started from before the system one, so a file of that name beside a downloaded tfg-gui.exe would have been loaded and run. It now comes from an absolute path under the system directory, asked for through kernel32, which is the one library that cannot be diverted. Checked on a real system rather than by reading: the guard that asks Windows for dark menus calls it twice and still gets them. That change moved the library's NAME out of the syscall call and into a helper of ours, where the no telemetry scan would have walked past it. One shared list of loading calls now covers both, and our helper is on it. A guard that gets safer code and stops looking is worse than the code it was guarding. S9 is accepted as a fact and refused as a remedy. The localiser is a package variable with no lock, written once before any screen exists, which is a constraint rather than a property of the code. An atomic pointer would be an eighth defence nothing in this build could redden - seven have been removed for that. The constraint is mechanical instead: a guard fails when a second caller appears, so the language switch starts from the sentence on the variable rather than discovering it. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 10 ++ internal/core/diskspace_windows.go | 23 +++ internal/guard/guitext_test.go | 4 +- internal/guard/hardening_test.go | 264 +++++++++++++++++++++++++++++ internal/guard/notelemetry_test.go | 107 ++++++++++-- internal/gui/darkmenus_windows.go | 65 ++++++- internal/gui/text/catalogue.go | 19 +++ 7 files changed, 467 insertions(+), 25 deletions(-) create mode 100644 internal/guard/hardening_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ee1951..1543063 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,16 @@ because it turns other people's test suites red. ### Security +- **On Windows, the desktop window loads the library it uses for dark menus from + the system directory rather than by name.** Asking for `uxtheme.dll` by name + goes through the standard Windows search order, and the directory the program + was started from comes before the system one in that order - so a file of that + name left beside a downloaded `tfg-gui.exe` would have been loaded into the + program and run. + + Nothing about the window changes. The menu is still dark, which was checked by + asking Windows on a real machine rather than by reading the code. + - **The password of a locked archive is no longer repeated in the recorded command line.** It appeared twice in the manifest: under the file's own `properties`, where it is written on purpose because a locked fixture nobody diff --git a/internal/core/diskspace_windows.go b/internal/core/diskspace_windows.go index 50c81cc..90de3bb 100644 --- a/internal/core/diskspace_windows.go +++ b/internal/core/diskspace_windows.go @@ -49,6 +49,29 @@ func AvailableBytes(path string) (int64, error) { // already in the graph as an indirect one, so promoting it would not add a // download - but it would put it inside the command line binary, which does not // link it today, and that is untouchable rule 11 rather than tidying. +// NewLazyDLL goes through the standard search order, which has historically +// included the directory the program was started from, and an outside review on +// 2026-09-05 asked for NewLazySystemDLL instead - which asks for System32 and +// nothing else. Two measurements settle why it stays as it is. +// +// THAT FUNCTION IS NOT IN THE STANDARD LIBRARY. The review said it was. +// Measured on 2026-09-06: syscall offers LoadDLL, LoadLibrary and NewLazyDLL +// and no System variant, no LoadLibraryEx and no LOAD_LIBRARY_SEARCH_ constants +// - the compiler says "undefined: syscall.NewLazySystemDLL". The System form +// lives in golang.org/x/sys/windows, and putting that inside the command line +// binary is the thing the paragraph above turns down under untouchable rule 11. +// +// AND THIS LOOKUP NEVER CONSULTS THE SEARCH ORDER. kernel32.dll is a KnownDLL: +// measured in the registry on this machine, HKLM\SYSTEM\CurrentControlSet\ +// Control\Session Manager\KnownDLLs holds 37 entries and one of them is +// "*kernel32 = kernel32.dll". A KnownDLL is already mapped, so the loader hands +// back the module that is there and there is nothing to put in front of it. +// +// What the review was right about is the next call rather than this one, and +// that part is now mechanical instead of remembered: a guard allows this form +// only for names on the KnownDLLs list, so a second NewProc pointed at an +// ordinary library is a failure with a sentence rather than a pattern inherited +// from the line above it. var ( kernel32 = syscall.NewLazyDLL("kernel32.dll") getDiskFree = kernel32.NewProc("GetDiskFreeSpaceExW") diff --git a/internal/guard/guitext_test.go b/internal/guard/guitext_test.go index dcf600e..455a4cd 100644 --- a/internal/guard/guitext_test.go +++ b/internal/guard/guitext_test.go @@ -60,7 +60,9 @@ var notWords = map[string]string{ `"files"`: "the group name a fresh screen starts at, and a recipe value", `"tfg-gui"`: "recorded in the manifest as the command that ran, a contract value", `"chickpea.png"`: "the name the toolkit files the icon resource under, never shown", - `"preset"`: "the key the preset field is registered under, not a label", + `"GetSystemDirectoryW"`: "the Windows entry point that says where the system keeps its own " + + "libraries, asked for by name because that is how the loader takes it", + `"preset"`: "the key the preset field is registered under, not a label", `"outputDirectory"`: "the name the window files the last output directory under, never shown. " + "Translating a storage key would lose what was kept the day somebody changed language", `"windowWidth"`: "the name the window files its width under, never shown", diff --git a/internal/guard/hardening_test.go b/internal/guard/hardening_test.go new file mode 100644 index 0000000..58c2d89 --- /dev/null +++ b/internal/guard/hardening_test.go @@ -0,0 +1,264 @@ +package guard + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "strings" + "testing" +) + +// A library is loaded lazily only when the system already has it mapped. +// +// syscall.NewLazyDLL goes through the standard search order, which has +// historically included the directory the program was started from. An outside +// review on 2026-09-05 asked for syscall.NewLazySystemDLL instead. Two things +// were measured on 2026-09-06 and they point the same way: +// +// - THAT FUNCTION DOES NOT EXIST. syscall offers LoadDLL, LoadLibrary and +// NewLazyDLL and no System variant, no LoadLibraryEx, and no +// LOAD_LIBRARY_SEARCH_ constants. It lives in golang.org/x/sys/windows, +// which untouchable rule 11 keeps out of the command line binary - the +// comment beside the call has said so since 2026-08-25. +// - THE ONE CALL WE MAKE NEVER REACHES THE SEARCH ORDER. kernel32.dll is a +// KnownDLL - measured in the registry, 37 entries, one of them +// "*kernel32 = kernel32.dll" - so it is already mapped and the loader hands +// back what is there. +// +// So the finding is right about the NEXT call rather than about this one, and +// that is what this guard holds: this form is allowed for a KnownDLL and for +// nothing else. A second NewProc pointed at an ordinary library fails here with +// a sentence instead of inheriting the shape of the line above it. +// +// Read from the source rather than by calling anything, because the file is +// built only on Windows and this guard has to mean the same thing on the +// runners that are not. +func TestALibraryIsOnlyLoadedLazilyWhenTheSystemAlreadyHasIt(t *testing.T) { + // The names this form may be used for, and why. Windows keeps these mapped + // from boot, so the search order is never consulted for them. Measured + // rather than remembered: the KnownDLLs registry key on 2026-09-06. + knownDLLs := map[string]string{ + "kernel32.dll": "a KnownDLL, always already mapped - free space asks it for GetDiskFreeSpaceExW", + } + + // Where a load may name something this cannot read, and why. A path worked + // out at run time is the SAFE form when it is absolute and comes from the + // system rather than from the search order, and it is the dangerous one + // when it comes from anywhere else - a list of names cannot tell those + // apart, so the file is named instead. + byPath := map[string]string{ + "internal/gui/darkmenus_windows.go": "builds an absolute path from the system directory, because uxtheme.dll is not a KnownDLL", + } + + root := repoRoot(t) + used := map[string]bool{} + viaPath := map[string]bool{} + + err := filepath.WalkDir(filepath.Join(root, "internal"), func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + rel := filepath.ToSlash(strings.TrimPrefix(path, root+string(filepath.Separator))) + if d.IsDir() { + if rel == "internal/guard" { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, 0) + if perr != nil { + return perr + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + if pkg, ok := sel.X.(*ast.Ident); !ok || pkg.Name != "syscall" { + return true + } + switch sel.Sel.Name { + case "NewLazyDLL", "LoadDLL", "LoadLibrary": + default: + return true + } + name := loadedName(call) + where := fset.Position(call.Pos()).Line + if name == "" { + if _, granted := byPath[rel]; !granted { + t.Errorf("%s:%d loads a library under a name this guard cannot read.\n"+ + "A load whose argument is worked out at run time is safe when the path is "+ + "absolute and comes from the system, and unsafe when it comes from anywhere "+ + "else - and nothing here can tell those apart. Add this file to the list "+ + "above with the reason, or name a KnownDLL.", rel, where) + return true + } + viaPath[rel] = true + return true + } + if _, known := knownDLLs[name]; !known { + t.Errorf("%s:%d loads %q with syscall.%s.\n"+ + "That goes through the standard search order, which has included the directory "+ + "the program was started from, and only a KnownDLL is immune because it is "+ + "already mapped. syscall has no System variant of this call - measured 2026-09-06 - "+ + "so a library that is not on the list above needs a deliberate answer rather than "+ + "this form: an absolute path, or golang.org/x/sys/windows with the owner's yes "+ + "under untouchable rule 11.", rel, where, name, sel.Sel.Name) + return true + } + used[name] = true + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walking internal: %v", err) + } + + if len(used) == 0 { + t.Fatal("no library lookup was found at all, so this guard checked nothing. " + + "internal/core/diskspace_windows.go makes one - if it has gone, take this with it.") + } + // An allowance that has outlived its call is an allowance nobody granted. + for name, why := range knownDLLs { + if !used[name] { + t.Errorf("%s is allowed to be loaded this way (%s) and nothing loads it.\n"+ + "Delete the entry rather than leaving it to cover the next arrival.", name, why) + } + } + for rel, why := range byPath { + if !viaPath[rel] { + t.Errorf("%s is allowed to load a library by a path it works out (%s) and it loads "+ + "none.\nDelete the entry rather than leaving it to cover whatever lands in that "+ + "file next.", rel, why) + } + } +} + +// loadedName is the library a lazy load asks for, or the empty string when the +// argument is not a plain literal. +// +// Anything that is not a literal is reported as unnamed and refused by the +// caller, which is the answer that cannot be wrong: a name worked out at run +// time is exactly the case a list of allowed names cannot judge. +func loadedName(call *ast.CallExpr) string { + if len(call.Args) != 1 { + return "" + } + lit, ok := call.Args[0].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return "" + } + return strings.Trim(lit.Value, `"`) +} + +// The text catalogue is loaded from exactly one place. +// +// internal/gui/text keeps the localiser in a package variable with no lock: +// Load writes it, and say, sayf and sayN read it on every string the window +// draws. That is safe because it happens once, in run, before the first screen +// exists - and that is a constraint rather than a property of the code. +// +// A second caller makes it a write racing every read on the interface thread. A +// language switch is the obvious one and LoadBuiltIn's own comment names it as +// its own piece of work. The race detector would not necessarily say so, because +// fyne.Do runs on the calling goroutine under the test driver, which is exactly +// the condition that hides threading defects in this tree. +// +// So the constraint is held here rather than defended with an atomic pointer +// nothing in this build could redden - a shape this project removes rather than +// keeps. Whoever adds the language switch meets this guard and starts from the +// sentence on the variable. Raised by an outside review on 2026-09-05. +func TestTheTextCatalogueIsLoadedFromOnePlaceOnly(t *testing.T) { + root := repoRoot(t) + + // Where the one call is allowed to be, and why. An entry naming a file that + // no longer calls it is a failure below rather than a comment nobody reads. + allowed := map[string]string{ + "internal/gui/run_cgo.go": "before the first screen is built, which is what makes a plain variable enough", + } + + found := map[string]int{} + err := filepath.WalkDir(filepath.Join(root, "internal"), func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + rel := filepath.ToSlash(strings.TrimPrefix(path, root+string(filepath.Separator))) + if d.IsDir() { + switch rel { + case "internal/guard", "internal/gui/text": + // The package itself, where Load and LoadBuiltIn are declared + // and where LoadBuiltIn calls Load. + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + + fset := token.NewFileSet() + file, perr := parser.ParseFile(fset, path, nil, 0) + if perr != nil { + return perr + } + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + pkg, ok := sel.X.(*ast.Ident) + if !ok || pkg.Name != "text" { + return true + } + if sel.Sel.Name == "Load" || sel.Sel.Name == "LoadBuiltIn" { + found[rel]++ + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walking internal: %v", err) + } + + total := 0 + for rel, n := range found { + total += n + if _, granted := allowed[rel]; !granted { + t.Errorf("%s loads the text catalogue, and only one place may.\n"+ + "The localiser is a package variable with no lock, written by Load and read by "+ + "every string the window draws. A second caller is a write racing those reads on "+ + "the interface thread, and the race detector will not necessarily say so. If this "+ + "is the language switch, that work starts by making the localiser safe to replace - "+ + "see the comment on it.", rel) + } + } + if total != 1 { + t.Errorf("the text catalogue is loaded %d times and it has to be loaded exactly once: %v.\n"+ + "Once is what makes a variable with no lock enough.", total, found) + } + for rel, why := range allowed { + if found[rel] == 0 { + t.Errorf("%s is listed as the one place that loads the text catalogue (%s) and it does "+ + "not load it.\nDelete the entry, or move it to wherever the call went - a standing "+ + "exception for code that has gone quietly covers the next thing that lands there.", + rel, why) + } + } +} diff --git a/internal/guard/notelemetry_test.go b/internal/guard/notelemetry_test.go index 05b42e3..c8ee43a 100644 --- a/internal/guard/notelemetry_test.go +++ b/internal/guard/notelemetry_test.go @@ -22,10 +22,13 @@ import ( // // - An endpoint written into the code. A string is not an import, so a URL // sitting in a constant is invisible to both guards beside this one. -// - A library loaded by name. internal/gui reaches uxtheme.dll through -// syscall.NewLazyDLL and calls into it by ordinal, which is a documented and -// wanted thing - and it is also the exact shape that would load wininet.dll -// instead. This is Go's version of ctypes.windll. +// - A library loaded by name. internal/gui reaches uxtheme.dll and calls into +// it by ordinal, which is a documented and wanted thing - and it is also the +// exact shape that would load wininet.dll instead. This is Go's version of +// ctypes.windll. Since 2026-09-06 that load names an absolute path under the +// system directory rather than a bare file name, so the library's name moved +// into a helper of ours and the scan follows it there - see +// libraryLoadCalls. // - A socket opened under the import graph. syscall is legitimately imported // in eight shipped files for disk space, signals and dark menus, so banning // it is not available. Naming the socket shaped calls is. @@ -226,7 +229,44 @@ func telemetryFindings(src, rel string) []telemetryFinding { } return true }) - return found + return withoutRegisteredPathLoads(found, rel) +} + +// librariesLoadedByPath are the files allowed to name a library with something +// other than a literal, and why. +// +// A computed name is normally the worst answer this guard can get - it cannot +// read what is being loaded, so it cannot vouch for it. There is one case where +// it is the SAFER answer and this is it: uxtheme.dll is not a KnownDLL, so +// naming it plainly goes through the standard search order and the directory the +// program was started from comes first in that order. Building an absolute path +// under the system directory is what closes that, and an absolute path is by +// definition not a literal. +// +// Measured on 2026-09-06: the KnownDLLs registry key holds thirty seven entries, +// kernel32.dll is one of them and uxtheme.dll is not. +// +// The file is named rather than the shape, because the shape is exactly what +// this guard cannot tell apart. +var librariesLoadedByPath = map[string]string{ + "internal/gui/darkmenus_windows.go": "the dark window menu, loaded from an absolute path under the system directory " + + "because uxtheme.dll is not a KnownDLL - see systemLibraryPath there", +} + +// withoutRegisteredPathLoads drops the one finding a registered file is allowed +// to raise, and leaves every other finding from that file alone. +func withoutRegisteredPathLoads(found []telemetryFinding, rel string) []telemetryFinding { + if _, granted := librariesLoadedByPath[rel]; !granted { + return found + } + kept := make([]telemetryFinding, 0, len(found)) + for _, f := range found { + if f.kind == "library" && f.detail == computedLibraryName { + continue + } + kept = append(kept, f) + } + return kept } // lowLevelLocalNames maps the name a file uses for a low level package back to @@ -307,16 +347,16 @@ func urlFinding(lit *ast.BasicLit, rel string) []telemetryFinding { // callFindings reports a call that loads a library, opens a socket or starts a // program. func callFindings(call *ast.CallExpr, local map[string]bool) []telemetryFinding { + if libraryLoadCalls[libraryCallName(call)] { + return libraryFinding(call) + } + sel, ok := call.Fun.(*ast.SelectorExpr) if !ok { return nil } name := sel.Sel.Name - if name == "NewLazyDLL" || name == "NewLazySystemDLL" || name == "LoadDLL" || name == "LoadLibrary" { - return libraryFinding(call) - } - // A call on a name the file resolved to a low level package. The receiver // has to be that name, so an ordinary method called Send on our own type is // not mistaken for a socket. @@ -333,6 +373,45 @@ func callFindings(call *ast.CallExpr, local map[string]bool) []telemetryFinding return nil } +// computedLibraryName is what this guard says about a load whose argument it +// cannot read. Spelled once, because the check that raises it and the registry +// that forgives it in one named file have to mean the same string. +const computedLibraryName = "a library named by something other than a literal" + +// libraryLoadCalls are the calls that name a library, whoever owns them. +// +// The four from syscall are the obvious half. systemLibraryPath is ours and it +// is here for a reason worth writing down: on 2026-09-06 the window stopped +// asking for uxtheme.dll by bare name - which goes through the standard search +// order - and started building an absolute path under the system directory +// instead. That moved the library's NAME out of the syscall call and into a +// helper of our own, where every check in this file would have walked straight +// past it. A guard that gets safer code and stops looking is worse than the +// code it was guarding. +// +// One set rather than the two lists that used to say this, which were four +// names written twice. +var libraryLoadCalls = map[string]bool{ + "NewLazyDLL": true, "NewLazySystemDLL": true, "LoadDLL": true, "LoadLibrary": true, + "systemLibraryPath": true, +} + +// libraryCallName is the name of the function a call names, whether it is +// written as a package selector or as a plain function of ours. +// +// Not the calledName in interfacethread_test.go, which answers the empty string +// for a marshalling selector - that is right for the question it asks and would +// silently drop calls from this one. +func libraryCallName(call *ast.CallExpr) string { + switch fn := call.Fun.(type) { + case *ast.SelectorExpr: + return fn.Sel.Name + case *ast.Ident: + return fn.Name + } + return "" +} + // libraryFinding reads the library a load call names, and reports it when the // name is computed rather than written down - a library nobody can read here is // a library this guard cannot vouch for. @@ -342,7 +421,7 @@ func libraryFinding(call *ast.CallExpr) []telemetryFinding { } lit, ok := call.Args[0].(*ast.BasicLit) if !ok || lit.Kind != token.STRING { - return []telemetryFinding{{kind: "library", detail: "a library named by something other than a literal"}} + return []telemetryFinding{{kind: "library", detail: computedLibraryName}} } name, err := strconv.Unquote(lit.Value) if err != nil { @@ -452,13 +531,7 @@ func collectUses(src, rel string, urls, libraries map[string]bool) { // loadedLibrary reads the literal name out of a library load call. func loadedLibrary(call *ast.CallExpr) (string, bool) { - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok || len(call.Args) == 0 { - return "", false - } - switch sel.Sel.Name { - case "NewLazyDLL", "NewLazySystemDLL", "LoadDLL", "LoadLibrary": - default: + if len(call.Args) == 0 || !libraryLoadCalls[libraryCallName(call)] { return "", false } lit, ok := call.Args[0].(*ast.BasicLit) diff --git a/internal/gui/darkmenus_windows.go b/internal/gui/darkmenus_windows.go index 0cd227a..cc8377c 100644 --- a/internal/gui/darkmenus_windows.go +++ b/internal/gui/darkmenus_windows.go @@ -2,7 +2,11 @@ package gui -import "syscall" +import ( + "path/filepath" + "syscall" + "unsafe" +) // Asking Windows to draw this process's own menus dark. // @@ -54,14 +58,30 @@ const ( // telling anybody about at runtime: the menu stays the colour it already was, // which is exactly where this program was before any of this existed. func PreferDarkMenus() bool { - uxtheme := syscall.NewLazyDLL("uxtheme.dll") - if err := uxtheme.Load(); err != nil { + // By absolute path, and that is not tidiness. uxtheme.dll is NOT a + // KnownDLL - measured in the registry on 2026-09-06, thirty seven entries + // and it is not among them - so asking for it by name goes through the + // standard search order, and the directory the program was started from + // comes before System32 in that order. A file of that name left beside a + // downloaded tfg-gui.exe would be loaded into this process and run. + // + // The comment below used to say uxtheme is already loaded by any process + // that draws a window, which would make the lookup return the module that + // is there rather than search for one. That is probably true and it is an + // assumption rather than a measurement, and it is not one worth resting on + // when the answer costs a path join. + path, err := systemLibraryPath("uxtheme.dll") + if err != nil { + return false + } + uxtheme, err := syscall.LoadDLL(path) + if err != nil { return false } // Deliberately not freed. The setting is process wide and outlives this - // call, uxtheme is already loaded by any process that draws a window, and - // unloading a library the toolkit is using to draw would be a far worse - // thing to get wrong than a handle held for the life of the program. + // call, the toolkit is drawing with this library too, and unloading one it + // is using would be a far worse thing to get wrong than a handle held for + // the life of the program. // // Looked up through kernel32 rather than through a helper that takes an // ordinal, because the standard library has no such helper and the one that @@ -73,10 +93,41 @@ func PreferDarkMenus() bool { // than a pointer when the high word is zero, which is what MAKEINTRESOURCE // builds. 135 fits in the low word. getProcAddress := syscall.NewLazyDLL("kernel32.dll").NewProc("GetProcAddress") - addr, _, _ := getProcAddress.Call(uxtheme.Handle(), uintptr(uxthemeSetPreferredAppMode)) + addr, _, _ := getProcAddress.Call(uintptr(uxtheme.Handle), uintptr(uxthemeSetPreferredAppMode)) if addr == 0 { return false } _, _, _ = syscall.SyscallN(addr, uintptr(preferredAppModeForceDark)) return true } + +// systemLibraryPath is where Windows keeps its own libraries, with name on the +// end of it. +// +// Asked of the system rather than built from the SystemRoot environment +// variable, because an environment is something a parent process chooses and +// this exists to stop a library being loaded from somewhere somebody else +// chose. +// +// kernel32 is looked up by name here and that is safe where uxtheme was not: +// it is a KnownDLL, so it is already mapped and the loader hands back the +// module that is there without consulting any search order. Measured in the +// registry on 2026-09-06 - kernel32.dll is on that list and uxtheme.dll is not. +// +// syscall has neither NewLazySystemDLL nor LoadLibraryEx nor the +// LOAD_LIBRARY_SEARCH_ constants - measured the same day, the compiler calls +// each of them undefined - and the module that does have them is one this +// project does not depend on directly. So the directory is asked for and the +// path is joined, which needs nothing that is not already here. +func systemLibraryPath(name string) (string, error) { + getSystemDirectory := syscall.NewLazyDLL("kernel32.dll").NewProc("GetSystemDirectoryW") + buf := make([]uint16, syscall.MAX_PATH) + n, _, err := getSystemDirectory.Call(uintptr(unsafe.Pointer(&buf[0])), uintptr(len(buf))) + // Nought is the failure, and a count larger than the buffer means it did + // not fit - neither answer is a directory, and guessing one would put this + // back where it started. + if n == 0 || int(n) > len(buf) { + return "", err + } + return filepath.Join(syscall.UTF16ToString(buf[:n]), name), nil +} diff --git a/internal/gui/text/catalogue.go b/internal/gui/text/catalogue.go index 9a73a92..4bfe34c 100644 --- a/internal/gui/text/catalogue.go +++ b/internal/gui/text/catalogue.go @@ -51,6 +51,25 @@ var builtIn embed.FS // resolved rather than the newest, because moving it would move a dependency of // the toolkit and that is a question about byte stability rather than about // translations. See docs/STACK.md. +// WRITTEN ONCE, BEFORE ANY SCREEN EXISTS, AND NEVER AGAIN. That is what makes a +// plain variable enough here, and it is a constraint rather than an accident: +// Load writes it, say, sayf and sayN read it on every string the window draws, +// and there is no lock between them. +// +// LoadBuiltIn is called from one place, in run before the first screen is +// built, and a guard holds it there. The moment something calls Load a second +// time - a language switch is the obvious one, and LoadBuiltIn's own comment +// names it as its own piece of work - this becomes a write racing every read on +// the interface thread. The race detector will not necessarily say so either, +// because fyne.Do runs on the calling goroutine under the test driver, which is +// exactly the condition that hides threading defects in this tree. +// +// An atomic pointer would make the write safe and would be a defence nothing in +// this build can redden, which is a shape this project takes out rather than +// keeps - seven such pieces have gone. So the constraint is mechanical instead: +// the guard fails when a second caller appears, and whoever adds the language +// switch starts from this sentence rather than discovering it. Raised by an +// outside review on 2026-09-05. var localiser *i18n.Localizer // sayf is say for a sentence that has values in it.