From aa69aaa26b5640cb80621c97b7145b3d4bcae0dd Mon Sep 17 00:00:00 2001 From: bartvanbos Date: Thu, 5 Nov 2026 13:28:12 +0100 Subject: [PATCH 1/3] chore: Go 1.26 floor, go fix, workflows v3.3.1 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 2 +- CONTRIBUTING.md | 4 ++-- README.md | 6 +++--- exit.go | 3 +-- go.mod | 2 +- main.go | 2 +- main_test.go | 8 ++++---- serve_test.go | 8 ++++---- 8 files changed, 17 insertions(+), 18 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 779ad34..1530172 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,7 +19,7 @@ jobs: coverage-min: "90" fuzz-time: 10s codecov-name: sunspecctl - go-versions: '["1.23", "1.24", "1.25", "1.26"]' + go-versions: '["1.26", "1.27"]' test-exclude-regex: "/example(s)?($|/)" secrets: inherit diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5ac7497..f35f6c2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -19,7 +19,7 @@ Thank you for your interest in contributing to `sunspecctl`! This document cover ### Prerequisites -- Go 1.23+ (see [go.mod](go.mod)); the code must build and pass on Go 1.23, so do not use newer language or standard-library features +- Go 1.26+ (see [go.mod](go.mod)); the code must build and pass on Go 1.26, so do not use newer language or standard-library features - [staticcheck](https://staticcheck.dev/), [golangci-lint](https://golangci-lint.run/) and [govulncheck](https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck) for the checks - No hardware: tests use the SunSpec fixture server of go-sunspec's [`testutil`](https://pkg.go.dev/github.com/otfabric/go-sunspec/testutil) package, and `serve` itself running in-process @@ -83,7 +83,7 @@ Examples: - Use go-sunspec's `testutil` package for anything that talks to a device: it serves a SunSpec model chain over Modbus TCP. For `serve`, start the command in-process on a free port (`startServe` in `serve_test.go`) and read it back with the other commands - A change to `capture`, `serve` or `template`, or a go-sunspec update, must keep `TestCaptureServeReproducesTheDevice` meaningful: capture → serve → read gives the same decoded values - Assert behaviour: the printed output, the JSON document, the returned error — not just that the code ran -- Keep tests fast, deterministic and race-clean; CI runs them with `-race` on every Go version from 1.23 and a 120 s per-package timeout +- Keep tests fast, deterministic and race-clean; CI runs them with `-race` on Go 1.26 and 1.27 and a 120 s per-package timeout - Coverage is enforced by `make check` and Codecov; new code should come with tests - If your change affects what is read from a device, verify against real hardware when possible and say which device in the pull request diff --git a/README.md b/README.md index d073eb7..d221d51 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # sunspecctl — SunSpec Modbus Command-Line Tool for Solar Inverters, Meters and Batteries -[![Go](https://img.shields.io/badge/Go-1.23%2B-00ADD8?style=flat&logo=go)](https://go.dev/) +[![Go](https://img.shields.io/badge/Go-1.26%2B-00ADD8?style=flat&logo=go)](https://go.dev/) [![License](https://img.shields.io/badge/License-MIT-yellow.svg)](LICENSE) [![CI](https://github.com/otfabric/sunspecctl/actions/workflows/ci.yml/badge.svg)](https://github.com/otfabric/sunspecctl/actions/workflows/ci.yml) [![Code Coverage](https://codecov.io/gh/otfabric/sunspecctl/graph/badge.svg)](https://codecov.io/gh/otfabric/sunspecctl) @@ -182,7 +182,7 @@ Run the same commands again. They always fetch the latest release and overwrite ### With Go -If you have Go 1.23 or later, you can build and install the latest release straight from source: +If you have Go 1.26 or later, you can build and install the latest release straight from source: ```bash go install github.com/otfabric/sunspecctl@latest @@ -477,7 +477,7 @@ Tests need no hardware: they run against the SunSpec fixture server of go-sunspe ## Requirements -- Go 1.23+ to build from source (not needed for the pre-built binaries) +- Go 1.26+ to build from source (not needed for the pre-built binaries) - [otfabric/go-sunspec](https://github.com/otfabric/go-sunspec) v0.6.0+ and [otfabric/go-modbus](https://github.com/otfabric/go-modbus) v1.5.0+ ## Contributing diff --git a/exit.go b/exit.go index adbb5e7..bdbd3ad 100644 --- a/exit.go +++ b/exit.go @@ -64,8 +64,7 @@ func exitCode(err error) int { errors.As(err, &ne) && ne.Timeout(): return exitTimeout } - var ee *exitError - if errors.As(err, &ee) { + if ee, ok := errors.AsType[*exitError](err); ok { return ee.code } switch { diff --git a/go.mod b/go.mod index 63ac446..cf17ad0 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/otfabric/sunspecctl -go 1.23 +go 1.26.0 require ( github.com/otfabric/go-modbus v1.10.6 diff --git a/main.go b/main.go index bf74468..21660a1 100644 --- a/main.go +++ b/main.go @@ -457,7 +457,7 @@ func readPointCmd() *cobra.Command { // --- output helpers --- // printJSON writes v to out as indented JSON followed by a newline. -func printJSON(out io.Writer, v interface{}) error { +func printJSON(out io.Writer, v any) error { enc := json.NewEncoder(out) enc.SetIndent("", " ") return enc.Encode(v) diff --git a/main_test.go b/main_test.go index de48f1a..d49ff30 100644 --- a/main_test.go +++ b/main_test.go @@ -122,7 +122,7 @@ func newModel(t *testing.T, id uint16, instances int) *modelBuilder { } b := &modelBuilder{t: t, meta: meta} b.fill(meta.Group) - for i := 0; i < instances; i++ { + for range instances { b.fill(meta.Group.Groups[0]) } b.regs[0] = id @@ -145,7 +145,7 @@ func (b *modelBuilder) fill(g *registry.GroupMeta) int { func (b *modelBuilder) add(path string, vals map[string][]uint16) *modelBuilder { b.t.Helper() g := b.meta.Group - for _, name := range strings.Split(path, "/") { + for name := range strings.SplitSeq(path, "/") { if g = g.Group(name); g == nil { b.t.Fatalf("model %d has no group %s", b.meta.ID, path) } @@ -367,7 +367,7 @@ func wantFail(t *testing.T, r result, msg string) { } // decodeJSON decodes stdout, which must hold exactly one JSON document. -func decodeJSON(t *testing.T, r result, v interface{}) { +func decodeJSON(t *testing.T, r result, v any) { t.Helper() if r.err != nil { t.Fatalf("unexpected error: %v\nstderr: %s", r.err, r.stderr) @@ -1293,7 +1293,7 @@ func TestPrintDecodedModelRawWrapsAtSixteen(t *testing.T) { } func TestPrintJSONReportsEncodingErrors(t *testing.T) { - if err := printJSON(io.Discard, map[string]interface{}{"f": func() {}}); err == nil { + if err := printJSON(io.Discard, map[string]any{"f": func() {}}); err == nil { t.Error("printJSON of an unencodable value returned no error") } var out bytes.Buffer diff --git a/serve_test.go b/serve_test.go index 56b0f91..0c54325 100644 --- a/serve_test.go +++ b/serve_test.go @@ -263,7 +263,7 @@ func TestServeExampleDevice(t *testing.T) { // without depending on its column widths. func normalizeTable(s string) string { var out []string - for _, line := range strings.Split(s, "\n") { + for line := range strings.SplitSeq(s, "\n") { out = append(out, strings.Join(strings.Fields(line), " ")) } return strings.Join(out, "\n") @@ -329,11 +329,11 @@ func TestReadmeDeviceFile(t *testing.T) { t.Fatal(err) } text := string(readme) - start := strings.Index(text, "```json\n") - if start < 0 { + _, after, ok := strings.Cut(text, "```json\n") + if !ok { t.Fatal("README.md has no JSON example") } - example := text[start+len("```json\n"):] + example := after example = example[:strings.Index(example, "```")] r := runCLI(t, "serve", "--check", "--file", writeFile(t, example), "--url", "tcp://127.0.0.1:1502") if r.err != nil { From a11526cc7d937e9f4201028d395cfe1b84143b45 Mon Sep 17 00:00:00 2001 From: bartvanbos Date: Thu, 5 Nov 2026 14:41:47 +0100 Subject: [PATCH 2/3] fix: hostile-device and hostile-input hardening - Text output, serve's write log and error messages escape control characters, escape sequences, bidi overrides and invalid UTF-8 from device data (a device could clear the screen or forge output lines). - capture/template -o write 0600 via temp file + rename and refuse symlinks and non-regular files (exit 10); the old file survives. - serve --file is capped at 16 MiB and TLS files at 4 MiB (exit 3). - poll, poll-model and poll-point stop on interrupt during discovery. - Text write errors exit 10; non-positive --timeout/--interval, negative --count and --unit-id 0 are usage errors (exit 2). - detect docs match the code (exit 1 without the SunS marker). - Hostile device/input tests and two fuzz targets. Co-Authored-By: Claude Opus 5.5 --- README.md | 10 +- SECURITY.md | 3 +- capture.go | 10 +- docs/exitcodes.md | 8 +- docs/json-output.md | 6 +- files.go | 85 +++ fuzz_hostile_test.go | 84 +++ hostile_device_test.go | 495 ++++++++++++++++++ hostile_input_test.go | 341 ++++++++++++ main.go | 72 ++- output.go | 119 +++++ poll.go | 51 +- serve.go | 32 +- testdata/fuzz/FuzzTermSafe/seed-c1-bidi | 2 + testdata/fuzz/FuzzTermSafe/seed-forged-line | 2 + .../fuzz/FuzzTextOutput/seed-common-escapes | 3 + tls.go | 20 +- 17 files changed, 1285 insertions(+), 58 deletions(-) create mode 100644 files.go create mode 100644 fuzz_hostile_test.go create mode 100644 hostile_device_test.go create mode 100644 hostile_input_test.go create mode 100644 output.go create mode 100644 testdata/fuzz/FuzzTermSafe/seed-c1-bidi create mode 100644 testdata/fuzz/FuzzTermSafe/seed-forged-line create mode 100644 testdata/fuzz/FuzzTextOutput/seed-common-escapes diff --git a/README.md b/README.md index d221d51..e6f229a 100644 --- a/README.md +++ b/README.md @@ -203,8 +203,8 @@ The existing commands, their flags and their output are unchanged. | Flag | Default | Description | |------|---------|-------------| | `--url` | `tcp://localhost:502` | Modbus device URL | -| `--unit-id` | `1` | Modbus unit ID | -| `--timeout` | `10s` | Operation timeout | +| `--unit-id` | `1` | Modbus unit ID, 1–255 | +| `--timeout` | `10s` | Operation timeout, a positive duration | | `--json` | `false` | Output as JSON | | `--raw` | `false` | Include raw register hex in output | | `--baud` | `0` (19200) | Serial baud rate, for `rtu://` and `ascii://` URLs | @@ -252,8 +252,8 @@ The `poll`, `poll-model`, and `poll-point` commands work like their `read` count | Flag | Default | Description | |------|---------|-------------| -| `--interval` | `30s` | Interval between polls (supports `s`, `m`, `h`) | -| `--count` | `0` | Number of polls (`0` = infinite, stop with Ctrl-C) | +| `--interval` | `30s` | Interval between polls, a positive duration (supports `ms`, `s`, `m`, `h`) | +| `--count` | `0` | Number of polls (`0` = infinite, stop with Ctrl-C); not negative | ```bash # Poll all models every 10 seconds @@ -379,6 +379,8 @@ sunspecctl serve --file device.json --check `capture` writes models that decode cleanly as point values, which are easy to read and edit. A model without a known schema, or one that does not match its schema exactly (shorter, longer, a value that would not read back the same), is written as `registers`. Either way, serving the file gives a device that reads the same as the original: the same points, values, scale factors and not-implemented points at the same addresses. With `--raw`, every model is written as `registers` and the copy is register-for-register identical. `capture` writes enum and bitfield points as numbers, so that values the model definition does not name survive. +With `-o`, the file is written with mode 0600 to a temporary file next to it and then renamed into place, so an existing file is replaced whole or not at all. `-o` refuses a symbolic link and anything that is not a regular file (a directory, `/dev/null`); leave out `-o` to write to standard output. A device file `serve` reads may be at most 16 MiB. + A capture is a snapshot: the served values do not change by themselves. It can contain serial numbers and network addresses of the device; look before you share it. ### Unit IDs diff --git a/SECURITY.md b/SECURITY.md index 94094c8..fa4979d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -35,6 +35,7 @@ SunSpec devices are read, and with `sunspecctl serve` simulated, over Modbus, wh ### Data Read From Devices Is Untrusted Input - Model IDs, lengths, strings and values come from the device. Discovery is bounded and lengths are validated before decoding, but treat what `sunspecctl` prints as untrusted: validate ranges before using values for control decisions, and escape strings (manufacturer, model, serial number) before putting them in HTML, SQL or shell commands +- Text output and error messages escape what a terminal would act on in device data (control characters and escape sequences, bidirectional overrides, bytes that are not UTF-8), so a device cannot clear the screen, set the window title or forge output lines. JSON output keeps the strings as sent, JSON-escaped - `sunspecctl` output can contain serial numbers, network addresses and production figures; treat it as sensitive operational data when storing or sharing it ### Reading Only, as a Client @@ -47,7 +48,7 @@ As a client `sunspecctl` reads and decodes; no command writes SunSpec control po - **Listen on the loopback address unless others must reach it**: `--url tcp://127.0.0.1:1502`. `tcp://0.0.0.0:...` and `tcp://[::]:...` accept connections from every network the machine is on. Never expose a served device to the Internet - **Do not run a simulator on a production network** where a controller or a monitoring system could take it for real equipment: a device file made with `capture` carries the manufacturer, model and serial number of the original -- **A captured device file is operational data**: it can contain serial numbers, network addresses, settings and production figures. Check it before sharing or committing it +- **A captured device file is operational data**: it can contain serial numbers, network addresses, settings and production figures. `capture -o` writes it with mode 0600 and never through a symbolic link. Check it before sharing or committing it - Device files are parsed and validated before anything is served, but only serve files you wrote or reviewed ### Dependencies diff --git a/capture.go b/capture.go index 6525ee8..88e5afb 100644 --- a/capture.go +++ b/capture.go @@ -103,10 +103,16 @@ The file goes to standard output, or to the file named with --output.`, } // writeDeviceFile writes the file as indented JSON, to the named file or, -// when path is empty, to out. +// when path is empty, to out. A named file is written as writeFileAtomic +// does: mode 0600, since a capture holds serial numbers and addresses, never +// through a symbolic link, and never half. func writeDeviceFile(out io.Writer, path string, file *devicefile.File) error { if path == "" { return file.Write(out) } - return file.WriteFile(path) + data, err := file.Marshal() + if err != nil { + return err + } + return writeFileAtomic(path, data) } diff --git a/docs/exitcodes.md b/docs/exitcodes.md index 161f079..0d19297 100644 --- a/docs/exitcodes.md +++ b/docs/exitcodes.md @@ -5,15 +5,15 @@ sunspecctl's exit codes are a script-facing contract (`exit.go`). They follow th | Code | Kind | Meaning | |------|------|---------| | 0 | ok | Success. `poll` and `serve` also exit 0 after SIGINT or SIGTERM | -| 1 | findings | The device answered and is not a SunSpec device (`detect` itself reports this as a result and exits 0) | -| 2 | usage | Unknown command or flag, missing argument, invalid serial setting, an invalid `SUNSPECCTL_FORMAT` | -| 3 | invalid_input | An invalid URL, device file, TLS file, unknown point name, or a model without a schema for `template` | +| 1 | findings | The device answered and is not a SunSpec device: no probed base address holds the "SunS" marker. This includes `detect`, which then prints no result | +| 2 | usage | Unknown command or flag, missing argument, invalid serial setting, an invalid `SUNSPECCTL_FORMAT`, a `--timeout` or `--interval` that is not positive, a negative `--count`, `--unit-id 0` for a client command | +| 3 | invalid_input | An invalid URL, device file, TLS file (also one over its size limit: 16 MiB for a device file, 4 MiB for a PEM file), unknown point name, or a model without a schema for `template` | | 4 | transport | Connecting failed or the connection was lost; `serve` cannot listen | | 5 | timeout | `--timeout` passed | | 6 | protocol | A Modbus exception, a malformed frame, registers that do not decode as SunSpec | | 8 | unsupported | The device does not have the requested model | | 9 | permission | The operating system refused an operation | -| 10 | internal | The output could not be written, or something unexpected happened | +| 10 | internal | The output could not be written (standard output, text or JSON, or the `-o` file of `capture` and `template`, also when it is a symbolic link or not a regular file), or something unexpected happened | | 130 | interrupted | SIGINT or SIGTERM before a result | Exit 7 (partial) is not used. diff --git a/docs/json-output.md b/docs/json-output.md index b44c64a..3391787 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -48,7 +48,7 @@ With `read` and `poll`, a model that cannot be read gives `warning: partial read } ``` -`Detected: false` is a result: `detect` exits 0. `RegType` is go-modbus's number for the table, with 0 for holding registers. `Marker` holds the two registers of "SunS". There is one `Attempts` entry per probed base address; `error` is its only lower-case field, and is left out when the read worked. +`detect` prints this document only when it found the marker, so `Detected` is always true. A device that answers but has no marker at any probed address is not a result: `detect` prints nothing on stdout, the error on stderr, and exits 1 ([exit codes](exitcodes.md)). `RegType` is go-modbus's number for the table, with 0 for holding registers. `Marker` holds the two registers of "SunS". There is one `Attempts` entry per probed base address; `error` is its only lower-case field, and is left out when the read worked. ## Discovery @@ -110,6 +110,10 @@ With `read` and `poll`, a model that cannot be read gives `warning: partial read `RawValue` is the value in the registers, with the type of `Type`: a number, or a string for string points. `ScaledValue` is `RawValue` times 10 to the power of `SFRawValue`, and null when that does not apply: no scale factor, or a point that is not implemented. `Implemented` is false when the device reports the SunSpec "not implemented" value, and `RawValue` then holds that value (-32768 for `int16`). `Symbols` lists the names of the set bits or of the enum value, or is null. +JSON has no NaN or infinity: a float `RawValue`, or a `ScaledValue`, that is not a finite number is null. A float point the device reports as NaN is SunSpec's "not implemented" (`Implemented` false); an infinite one keeps `Implemented` true with a null `RawValue`. The model's `RawRegisters` hold the registers as read. + +Strings are written as the device sent them, escaped as JSON escapes them. Text output escapes what a terminal would act on instead: control characters as `\x1b`, `\t`, `\n`, `\r`, other non-printable characters such as bidirectional overrides as `\u202e`, and bytes that are not UTF-8 as `\xff`. Error messages on stderr are escaped the same way. + ## Served device ```json diff --git a/files.go b/files.go new file mode 100644 index 0000000..a021ef5 --- /dev/null +++ b/files.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: MIT + +package main + +import ( + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" +) + +// Size limits for the files sunspecctl reads. They are far above what a +// real file needs and keep a wrong path (/dev/zero, a disk image) from +// exhausting memory. +const ( + // maxDeviceFileSize bounds a device file. A device with every register + // of the address space given as a number takes about 1 MiB. + maxDeviceFileSize = 16 << 20 + // maxPEMFileSize bounds a certificate, key or CA bundle. A system CA + // bundle is a few hundred KiB. + maxPEMFileSize = 4 << 20 +) + +// errTooLarge is returned by readFileLimited for a file over its limit. +var errTooLarge = errors.New("file too large") + +// readFileLimited reads the file at path, failing with errTooLarge without +// reading further when it holds more than limit bytes. Errors name the path, +// as os.ReadFile's do. +func readFileLimited(path string, limit int64) ([]byte, error) { + f, err := os.Open(filepath.Clean(path)) + if err != nil { + return nil, err + } + defer func() { _ = f.Close() }() + data, err := io.ReadAll(io.LimitReader(f, limit+1)) + if err != nil { + return nil, err + } + if int64(len(data)) > limit { + return nil, fmt.Errorf("%s: %w: more than %d MiB", path, errTooLarge, limit>>20) + } + return data, nil +} + +// writeFileAtomic writes data to path as a new file with mode 0600: to a +// temporary file in the same directory first, which is then renamed over +// path, so that path holds either its old content or all of data. It refuses +// a path that is a symbolic link or not a regular file: the rename would +// replace the link or the device node, not write to what it names. +func writeFileAtomic(path string, data []byte) error { + path = filepath.Clean(path) + switch fi, err := os.Lstat(path); { + case errors.Is(err, fs.ErrNotExist): + case err != nil: + return err + case fi.Mode()&fs.ModeSymlink != 0: + return fmt.Errorf("%s is a symbolic link: refusing to write through it", path) + case !fi.Mode().IsRegular(): + return fmt.Errorf("%s is not a regular file", path) + } + tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+".*.tmp") + if err != nil { + return err + } + _, err = tmp.Write(data) + if err == nil { + err = tmp.Sync() + } + if cerr := tmp.Close(); err == nil { + err = cerr + } + if err == nil { + err = rename(tmp.Name(), path) + } + if err != nil { + _ = os.Remove(tmp.Name()) + } + return err +} + +// rename is os.Rename; tests replace it to make the last step fail. +var rename = os.Rename diff --git a/fuzz_hostile_test.go b/fuzz_hostile_test.go new file mode 100644 index 0000000..0f266cd --- /dev/null +++ b/fuzz_hostile_test.go @@ -0,0 +1,84 @@ +// SPDX-License-Identifier: MIT + +package main + +import ( + "bytes" + "strings" + "testing" + "unicode" + "unicode/utf8" + + sunspec "github.com/otfabric/go-sunspec" + "github.com/otfabric/go-sunspec/registry" +) + +// wantNoControl fails when s holds anything a terminal acts on, other than +// the newlines and the tabs the table layout writes itself when allowTab. +func wantNoControl(t *testing.T, s string, allowTab bool) { + t.Helper() + if !utf8.ValidString(s) { + t.Fatalf("output is not UTF-8: %q", s) + } + for _, r := range s { + if r == '\n' || allowTab && r == '\t' { + continue + } + if r != ' ' && !unicode.IsPrint(r) { + t.Fatalf("output holds %U: %q", r, s) + } + } +} + +// FuzzTermSafe checks the escaping of device strings for the terminal: the +// result holds printable runes only, and a printable string is left alone. +func FuzzTermSafe(f *testing.F) { + for _, s := range []string{"", "Acme", "\x1b[2J", "\x1b]0;title\x07", "a\tb\nc\r", "\xff\xfe", "1\u202e2", "\u009b31m", "ü€😀", `\x41`, "\x00\x7f"} { + f.Add(s) + } + f.Fuzz(func(t *testing.T, s string) { + got := termSafe(s) + wantNoControl(t, got, false) + if utf8.ValidString(s) && !strings.ContainsFunc(s, func(r rune) bool { return r != ' ' && !unicode.IsPrint(r) }) && got != s { + t.Fatalf("printable %q changed to %q", s, got) + } + if len(got) < len(s) { + t.Fatalf("escaping lost bytes: %q -> %q", s, got) + } + }) +} + +// FuzzTextOutput decodes registers as a device reports them and prints them +// as the text commands do: whatever the device sends, the output must not +// hold a control sequence. +func FuzzTextOutput(f *testing.F) { + f.Add(uint16(1), []byte("\x00\x01\x00\x42\x1b[2J\x1b]0;x\x07\xff\xfe\n\tSN")) + f.Add(uint16(160), []byte{0x00, 0xA0, 0x00, 0x30, 0x9b, 0x9b}) + f.Add(uint16(111), []byte{0x00, 0x6F, 0x00, 0x3C, 0x7F, 0xC0, 0, 0, 0x7F, 0x80, 0, 0}) + f.Fuzz(func(t *testing.T, id uint16, data []byte) { + meta := registry.ByID(id) + if meta == nil { + return + } + regs := make([]uint16, len(data)/2) + for i := range regs { + regs[i] = uint16(data[2*i])<<8 | uint16(data[2*i+1]) + } + dm, _ := sunspec.DecodeModel(regs, meta, 40000) + if dm == nil { + return + } + var out bytes.Buffer + printDecodedModel(&out, dm) + // printGroup's tabwriter pads with spaces; the only tabs left would + // be a device's. + wantNoControl(t, out.String(), false) + if dm.Group != nil { + for _, p := range dm.Group.Points { + out.Reset() + printPoint(&out, &p) + wantNoControl(t, out.String(), false) + } + } + }) +} diff --git a/hostile_device_test.go b/hostile_device_test.go new file mode 100644 index 0000000..aadc68b --- /dev/null +++ b/hostile_device_test.go @@ -0,0 +1,495 @@ +// SPDX-License-Identifier: MIT + +package main + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io" + "math" + "net" + "strings" + "sync" + "testing" + "time" + + "github.com/otfabric/go-sunspec" + "github.com/otfabric/go-sunspec/testutil" +) + +// Tests in this file play a hostile or broken device: what it reports is +// untrusted input, and every command must end with a clear error and the +// documented exit code, in bounded time and without a panic. + +// f32 returns the two registers of a float32. +func f32(v float32) []uint16 { + b := math.Float32bits(v) + return []uint16{uint16(b >> 16), uint16(b)} +} + +// floatMeter returns a device with a float model (111, single phase +// inverter, float) whose points hold NaN, +Inf and -Inf, as a device may +// report them. NaN is also SunSpec's "not implemented" value for floats, so +// an ordinary device reports it too. +func floatMeter(t *testing.T) *testutil.SunSpecHandler { + t.Helper() + common := newModel(t, 1, 0).str("Mn", "Acme") + inv := newModel(t, 111, 0). + set("A", f32(float32(math.NaN()))...). + set("AphA", f32(float32(math.Inf(1)))...). + set("PhVphA", f32(float32(math.Inf(-1)))...). + set("Hz", f32(50.01)...) + return testutil.NewSunSpecFixture(40000, 1, common.fixture(), inv.fixture()) +} + +// jsonPoints indexes the decoded points of a JSON model document by name, +// as generic JSON. +func jsonPoints(t *testing.T, model map[string]any) map[string]map[string]any { + t.Helper() + group, ok := model["Group"].(map[string]any) + if !ok { + t.Fatalf("model without group: %v", model) + } + out := map[string]map[string]any{} + for _, p := range group["Points"].([]any) { + pm := p.(map[string]any) + out[pm["Name"].(string)] = pm + } + return out +} + +func TestHostileNonFiniteFloatsInJSON(t *testing.T) { + url := startDevice(t, floatMeter(t)) + + // read --json printed nothing and failed with "json: unsupported value: + // NaN" for any device with a float point that is not implemented. + r := runCLI(t, "read", "--json", "--url", url) + var models []map[string]any + decodeJSON(t, r, &models) + if len(models) != 2 { + t.Fatalf("%d models, want 2", len(models)) + } + pts := jsonPoints(t, models[1]) + // go-sunspec encodes a value that is not a finite number as null; the + // model's RawRegisters keep the bits. + for _, name := range []string{"A", "AphA", "PhVphA"} { + if v, ok := pts[name]["RawValue"]; !ok || v != nil { + t.Errorf("%s RawValue = %#v, want null", name, v) + } + } + if pts["A"]["Implemented"] != false || pts["AphA"]["Implemented"] != true { + t.Errorf("Implemented: A %v, AphA %v", pts["A"]["Implemented"], pts["AphA"]["Implemented"]) + } + // Finite values stay numbers, and the document keeps the field order. + if hz, ok := pts["Hz"]["RawValue"].(float64); !ok || math.Abs(hz-50.01) > 1e-4 { + t.Errorf("Hz RawValue = %#v", pts["Hz"]["RawValue"]) + } + + for _, args := range [][]string{ + {"read-model", "--id", "111"}, + {"poll-model", "--id", "111", "--count", "2", "--interval", "1ms"}, + {"poll", "--count", "1"}, + } { + r := runCLI(t, append(args, "--json", "--url", url)...) + if r.err != nil || jsonDocuments(t, r.stdout) == 0 { + t.Errorf("%v: err %v\n%s", args, r.err, r.stdout) + } + } + r = runCLI(t, "read-point", "--model", "111", "--point", "PhVphA", "--json", "--url", url) + var p map[string]any + decodeJSON(t, r, &p) + if v, ok := p["RawValue"]; !ok || v != nil || p["Implemented"] != true { + t.Errorf("read-point RawValue = %#v", p["RawValue"]) + } + + // Text output was fine already and stays so. + r = runCLI(t, "read-point", "--model", "111", "--point", "AphA", "--url", url) + if r.err != nil || !strings.Contains(r.stdout, "Raw: +Inf\n") { + t.Errorf("read-point text: %v\n%s", r.err, r.stdout) + } +} + +// jsonDocuments returns the number of JSON documents in s, which must hold +// nothing else. +func jsonDocuments(t *testing.T, s string) int { + t.Helper() + dec := json.NewDecoder(strings.NewReader(s)) + n := 0 + for { + var v any + err := dec.Decode(&v) + if errors.Is(err, io.EOF) { + return n + } + if err != nil { + t.Errorf("document %d: %v", n+1, err) + return 0 + } + n++ + } +} + +// A scale factor far outside SunSpec's -10..10 overflows the scaled value to +// infinity, or to NaN for a raw value of zero. +func TestHostileScaleFactorOverflow(t *testing.T) { + common := newModel(t, 1, 0).str("Mn", "Acme") + inv := newModel(t, 101, 0). + set("W", 5).set("W_SF", 0x7FFF). + set("A", 0).set("A_SF", 0x7FFF). + set("Hz", 5001).set("Hz_SF", 0x8001) // -32767 + url := startDevice(t, testutil.NewSunSpecFixture(40000, 1, common.fixture(), inv.fixture())) + + r := runCLI(t, "read-model", "--id", "101", "--json", "--url", url) + var model map[string]any + decodeJSON(t, r, &model) + pts := jsonPoints(t, model) + if pts["W"]["ScaledValue"] != nil || pts["A"]["ScaledValue"] != nil || pts["Hz"]["ScaledValue"] != float64(0) { + t.Errorf("ScaledValue: W %#v, A %#v, Hz %#v", pts["W"]["ScaledValue"], pts["A"]["ScaledValue"], pts["Hz"]["ScaledValue"]) + } + if pts["W"]["RawValue"] != float64(5) || pts["W"]["SFRawValue"] != float64(32767) { + t.Errorf("W: %v", pts["W"]) + } + r = runCLI(t, "read-point", "--model", "101", "--point", "W", "--url", url) + if r.err != nil || !strings.Contains(r.stdout, "Scaled: +Inf\n") { + t.Errorf("text: %v\n%s", r.err, r.stdout) + } +} + +// hostileStrings is a Common model whose strings carry terminal control +// sequences: clear screen, set the window title, a fake extra line, invalid +// UTF-8 and a right-to-left override. +func hostileStrings(t *testing.T) *testutil.SunSpecHandler { + t.Helper() + common := newModel(t, 1, 0). + str("Mn", "\x1b[2J\x1b]0;pwn\x07"). + str("Md", "a\tb\nFAKE line"). + str("Opt", "\xff\xfeok"). + str("Vr", "1\u202e2"). + str("SN", "\x9b31m") + return testutil.NewSunSpecFixture(40000, 1, common.fixture()) +} + +// wantTerminalSafe fails when s holds a control character other than a +// newline, or a Unicode format character. +func wantTerminalSafe(t *testing.T, what, s string) { + t.Helper() + for _, r := range s { + if r == '\n' { + continue + } + if r < 0x20 || r == 0x7F || (r >= 0x80 && r <= 0x9F) || r == '\u202e' || r == '\uFFFD' { + t.Errorf("%s holds %U:\n%q", what, r, s) + return + } + } +} + +func TestHostileStringsAreEscapedInText(t *testing.T) { + url := startDevice(t, hostileStrings(t)) + + r := runCLI(t, "read", "--url", url) + if r.err != nil { + t.Fatal(r.err) + } + wantTerminalSafe(t, "read", r.stdout) + for _, want := range []string{ + ` Mn string \x1b[2J\x1b]0;pwn\x07`, + ` Md string a\tb\nFAKE line`, + ` Opt string \xff\xfeok`, + ` Vr string 1\u202e2`, + ` SN string \x9b31m`, + } { + if !strings.Contains(r.stdout, want+"\n") { + t.Errorf("read lacks %q:\n%s", want, r.stdout) + } + } + if strings.Contains(r.stdout, "\nFAKE") { + t.Errorf("a device string started a new line:\n%s", r.stdout) + } + + r = runCLI(t, "read-point", "--model", "1", "--point", "Mn", "--url", url) + if r.err != nil || r.stdout != "Point: Mn\nType: string\nRaw: \\x1b[2J\\x1b]0;pwn\\x07\n" { + t.Errorf("read-point: %v\n%q", r.err, r.stdout) + } + r = runCLI(t, "poll-point", "--model", "1", "--point", "Md", "--count", "1", "--url", url) + wantTerminalSafe(t, "poll-point", r.stdout) + + // JSON keeps the strings as they are: encoding/json escapes the control + // characters itself, and decoding gives back the device's string. + r = runCLI(t, "read-model", "--id", "1", "--json", "--url", url) + var model map[string]any + decodeJSON(t, r, &model) + pts := jsonPoints(t, model) + for name, want := range map[string]string{"Mn": "\x1b[2J\x1b]0;pwn\x07", "Md": "a\tb\nFAKE line", "Vr": "1\u202e2"} { + if pts[name]["RawValue"] != want { + t.Errorf("JSON %s = %q, want %q", name, pts[name]["RawValue"], want) + } + } + if strings.ContainsAny(r.stdout, "\x1b\x07\t") { + t.Errorf("raw control characters in JSON:\n%q", r.stdout) + } +} + +func TestHostileStringsInServeWriteLog(t *testing.T) { + newRootCmd() // text output: reset --json of earlier tests + var out bytes.Buffer + log := writeLogger(&out) + log(&sunspec.ServerWrite{ClientAddr: "10.0.0.1:5", ModelID: 1, Address: 40002, Points: []sunspec.DecodedPoint{ + {Name: "Mn", RawValue: "\x1b[31mred\r\nwrite from 6.6.6.6: forged", Implemented: true}, + }}) + want := `write from 10.0.0.1:5: model 1 at 40002: Mn = \x1b[31mred\r\nwrite from 6.6.6.6: forged` + "\n" + if out.String() != want { + t.Errorf("got %q\nwant %q", out.String(), want) + } +} + +func TestHostileErrorMessagesAreEscaped(t *testing.T) { + // A device file names a point with an escape sequence: the error quotes + // the name, and the terminal must not act on it. + path := writeFile(t, `{"models": [{"id": 1, "points": {"\u001b]0;pwn\u0007": 1}}]}`) + r := runCLI(t, "serve", "--file", path, "--check") + if r.err == nil { + t.Fatal("accepted") + } + if code := exitCode(r.err); code != exitInvalidInput { + t.Errorf("exit %d, want %d", code, exitInvalidInput) + } + wantTerminalSafe(t, "stderr", r.stderr) + wantTerminalSafe(t, "error", r.err.Error()) + if !strings.Contains(r.stderr, `\x1b]0;pwn\x07`) { + t.Errorf("stderr:\n%s", r.stderr) + } +} + +// silentDevice accepts connections and reads requests but never answers. +func silentDevice(t *testing.T) string { + t.Helper() + return fakeDevice(t, func(c net.Conn) { + _, _ = io.Copy(io.Discard, c) + }) +} + +// fakeDevice serves each connection with handle on a local port and returns +// the URL. Connections are closed when the test ends. +func fakeDevice(t *testing.T, handle func(net.Conn)) string { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + var ( + mu sync.Mutex + conns []net.Conn + wg sync.WaitGroup + ) + wg.Go(func() { + for { + c, err := l.Accept() + if err != nil { + return + } + mu.Lock() + conns = append(conns, c) + mu.Unlock() + wg.Go(func() { handle(c) }) + } + }) + t.Cleanup(func() { + _ = l.Close() + mu.Lock() + for _, c := range conns { + _ = c.Close() + } + mu.Unlock() + wg.Wait() + }) + return "tcp://" + l.Addr().String() +} + +// runCancelled runs args against url and cancels the context after delay; it +// fails when the command does not return within limit. +func runCancelled(t *testing.T, delay, limit time.Duration, args ...string) (result, time.Duration) { + t.Helper() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + time.AfterFunc(delay, cancel) + done := make(chan result, 1) + start := time.Now() + go func() { done <- runCLIContext(t, ctx, args...) }() + select { + case r := <-done: + return r, time.Since(start) + case <-time.After(limit): + t.Fatalf("%v did not stop within %s of being interrupted", args, limit) + return result{}, 0 + } +} + +// Every command that talks to a device stops promptly when it is +// interrupted, also while the device keeps it waiting. The poll commands +// discovered with a context of their own, so Ctrl-C during discovery waited +// for --timeout to pass. +func TestHostileSilentDeviceInterrupt(t *testing.T) { + url := silentDevice(t) + for _, args := range [][]string{ + {"detect"}, {"models"}, {"read"}, {"read-model", "--id", "1"}, {"read-point", "--model", "1", "--point", "Mn"}, + {"capture"}, + {"poll"}, {"poll-model", "--id", "1"}, {"poll-point", "--model", "1", "--point", "Mn"}, + } { + t.Run(args[0], func(t *testing.T) { + r, took := runCancelled(t, 100*time.Millisecond, 5*time.Second, append(args, "--url", url, "--timeout", "1m")...) + // The poll commands end without error when interrupted (exit 0, + // docs/exitcodes.md); the others exit 130. + if strings.HasPrefix(args[0], "poll") { + if r.err != nil { + t.Errorf("err = %v after %s, want nil", r.err, took) + } + } else if code := exitCode(r.err); r.err == nil || code != exitInterrupted { + t.Errorf("exit %d (%v) after %s, want %d", code, r.err, took, exitInterrupted) + } + if r.stdout != "" && !strings.HasPrefix(r.stdout, "Usage:") { + t.Errorf("stdout: %q", r.stdout) + } + }) + } +} + +// A device that never answers ends a command with exit 5 after --timeout. +func TestHostileSilentDeviceTimeout(t *testing.T) { + url := silentDevice(t) + start := time.Now() + r := runCLI(t, "poll", "--count", "1", "--url", url, "--timeout", "300ms") + if code := exitCode(r.err); code != exitTimeout { + t.Errorf("exit %d (%v), want %d", code, r.err, exitTimeout) + } + if took := time.Since(start); took > 5*time.Second { + t.Errorf("took %s", took) + } +} + +// A device that drops the connection in the middle of a response ends the +// command with a transport error. +func TestHostileDeviceDropsConnection(t *testing.T) { + url := fakeDevice(t, func(c net.Conn) { + buf := make([]byte, 12) + if _, err := io.ReadFull(c, buf); err != nil { + return + } + // Half an MBAP header for the right transaction, then gone. + _, _ = c.Write(buf[:4]) + _ = c.Close() + }) + for _, args := range [][]string{{"detect"}, {"read"}, {"poll", "--count", "3", "--interval", "1ms"}} { + start := time.Now() + r := runCLI(t, append(args, "--url", url, "--timeout", "5s")...) + if r.err == nil { + t.Errorf("%v: no error", args) + continue + } + if code := exitCode(r.err); code != exitTransport && code != exitFindings && code != exitProtocol { + t.Errorf("%v: exit %d (%v)", args, code, r.err) + } + if took := time.Since(start); took > 4*time.Second { + t.Errorf("%v took %s", args, took) + } + } +} + +// A device that sends a response one byte at a time, slower than --timeout +// allows, is a timeout and not a hang. +func TestHostileTricklingDevice(t *testing.T) { + url := fakeDevice(t, func(c net.Conn) { + buf := make([]byte, 12) + if _, err := io.ReadFull(c, buf); err != nil { + return + } + resp := []byte{buf[0], buf[1], 0, 0, 0, 7, buf[6], 3, 4, 0x53, 0x75, 0x6E, 0x53} + for _, b := range resp { + if _, err := c.Write([]byte{b}); err != nil { + return + } + time.Sleep(200 * time.Millisecond) + } + }) + start := time.Now() + r := runCLI(t, "detect", "--url", url, "--timeout", "500ms") + if code := exitCode(r.err); r.err == nil || code != exitTimeout { + t.Errorf("exit %d (%v), want %d", code, r.err, exitTimeout) + } + if took := time.Since(start); took > 5*time.Second { + t.Errorf("took %s", took) + } +} + +// Broken model chains: a header whose length runs past the address space, a +// chain without an end, and a model that claims more registers than the +// device has. Each is a protocol error (exit 6), never a panic or a hang. +func TestHostileModelChains(t *testing.T) { + suns := map[uint16]uint16{40000: 0x5375, 40001: 0x6E53} + with := func(extra map[uint16]uint16) *testutil.SunSpecHandler { + regs := map[uint16]uint16{} + for k, v := range suns { + regs[k] = v + } + for k, v := range extra { + regs[k] = v + } + return &testutil.SunSpecHandler{UnitID: 1, Registers: regs} + } + endless := map[uint16]uint16{} + for a := uint16(40002); a < 40002+2*400; a += 2 { + endless[a], endless[a+1] = 64900, 0 + } + tests := []struct { + name string + h *testutil.SunSpecHandler + }{ + {"length past 0xFFFF", with(map[uint16]uint16{40002: 1, 40003: 0xFFFF})}, + {"no end marker", with(endless)}, + {"length longer than the device", with(map[uint16]uint16{40002: 1, 40003: 66, 40070: 0xFFFF, 40071: 0})}, + {"model ID 0", with(map[uint16]uint16{40002: 0, 40003: 0})}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + url := startDevice(t, tc.h) + for _, args := range [][]string{{"models"}, {"read", "--json"}, {"capture"}} { + start := time.Now() + r := runCLI(t, append(args, "--url", url, "--timeout", "5s")...) + if took := time.Since(start); took > 5*time.Second { + t.Errorf("%v took %s", args, took) + } + if r.err == nil { + // A device whose chain is readable but short is not an + // error; the output must then still be well formed. + if args[len(args)-1] == "--json" && !json.Valid([]byte(r.stdout)) { + t.Errorf("%v: invalid JSON", args) + } + continue + } + code := exitCode(r.err) + if code != exitProtocol && code != exitTransport { + t.Errorf("%v: exit %d (%v)", args, code, r.err) + } + } + }) + } +} + +// A bad marker is "not SunSpec": exit 1, or a result for detect. +func TestHostileBadMarker(t *testing.T) { + url := startDevice(t, &testutil.SunSpecHandler{UnitID: 1, Registers: map[uint16]uint16{ + 0: 0x5375, 1: 0x6E54, 40000: 0x0000, 40001: 0x0000, 50000: 0xFFFF, 50001: 0xFFFF, + }}) + r := runCLI(t, "models", "--url", url) + if !errors.Is(r.err, sunspec.ErrNotSunSpec) || exitCode(r.err) != exitFindings { + t.Errorf("models: %v", r.err) + } + // detect fails the same way. docs/exitcodes.md says it reports this as + // a result and exits 0; the code and TestNotSunSpec say exit 1. + r = runCLI(t, "detect", "--json", "--url", url) + if !errors.Is(r.err, sunspec.ErrNotSunSpec) || exitCode(r.err) != exitFindings || r.stdout != "" && !strings.HasPrefix(r.stdout, "Usage:") { + t.Errorf("detect: %v\n%s", r.err, r.stdout) + } +} diff --git a/hostile_input_test.go b/hostile_input_test.go new file mode 100644 index 0000000..8fc2c07 --- /dev/null +++ b/hostile_input_test.go @@ -0,0 +1,341 @@ +// SPDX-License-Identifier: MIT + +package main + +import ( + "bytes" + "errors" + "io/fs" + "os" + "path/filepath" + "runtime" + "strconv" + "strings" + "testing" + + "github.com/otfabric/go-sunspec/devicefile" +) + +// Tests in this file give the command line, and the files it reads and +// writes, values a user or a script should not have given. + +// Flag values that cannot be meant are usage errors, found before any I/O: +// they used to fail later and differently (--timeout 0 as a timeout, a +// negative --count as success without output, --interval 0 as polling as +// fast as the device answers, --unit-id 0 as a read of unit 1). +func TestHostileFlagValues(t *testing.T) { + tests := []struct { + args []string + want string + }{ + {[]string{"read", "--timeout", "0s"}, "invalid --timeout 0s: use a positive duration such as 10s"}, + {[]string{"detect", "--timeout", "-1s"}, "invalid --timeout -1s: use a positive duration such as 10s"}, + {[]string{"poll", "--count", "1", "--timeout", "0"}, "invalid --timeout 0s: use a positive duration such as 10s"}, + {[]string{"capture", "--timeout", "-5m"}, "invalid --timeout -5m0s: use a positive duration such as 10s"}, + {[]string{"poll", "--interval", "0s"}, "invalid --interval 0s: use a positive duration such as 5s"}, + {[]string{"poll-model", "--id", "1", "--interval", "-1s"}, "invalid --interval -1s: use a positive duration such as 5s"}, + {[]string{"poll-point", "--model", "1", "--point", "Mn", "--count", "-1"}, "invalid --count -1: use 0 (poll until interrupted) or more"}, + {[]string{"models", "--unit-id", "0"}, "invalid --unit-id 0: that is the Modbus broadcast address, which a device does not answer; use 1-255"}, + {[]string{"read-point", "--model", "1", "--point", "Mn", "--unit-id", "0"}, "invalid --unit-id 0: that is the Modbus broadcast address, which a device does not answer; use 1-255"}, + } + for _, tc := range tests { + t.Run(strings.Join(tc.args, " "), func(t *testing.T) { + // A URL nothing listens on: the error must come before any I/O. + r := runCLI(t, append(tc.args, "--url", "tcp://127.0.0.1:1")...) + wantFail(t, r, tc.want) + if code := exitCode(r.err); code != exitUsage { + t.Errorf("exit %d, want %d", code, exitUsage) + } + }) + } + + // serve takes --unit-id 0 from the device file's check. + r := runCLI(t, "serve", "--file", writeFile(t, exampleDevice), "--unit-id", "0", "--check") + if r.err == nil || exitCode(r.err) != exitInvalidInput || !strings.Contains(r.err.Error(), "broadcast") { + t.Errorf("serve --unit-id 0: %v", r.err) + } +} + +// Text output whose writes fail ends the command with exit 10, as JSON +// output did already; before, the command printed nothing and exited 0. +func TestHostileTextOutputWriteError(t *testing.T) { + url := startDevice(t, inverter(t)) + for _, args := range [][]string{ + {"version"}, {"detect"}, {"models"}, {"read"}, {"read-model", "--id", "101"}, + {"read-point", "--model", "101", "--point", "W"}, + {"poll", "--count", "3", "--interval", "1ms"}, + {"poll-model", "--id", "101", "--count", "3", "--interval", "1ms"}, + {"poll-point", "--model", "101", "--point", "W", "--count", "3", "--interval", "1ms"}, + {"serve", "--file", writeFile(t, exampleDevice), "--check"}, + } { + t.Run(args[0], func(t *testing.T) { + root := newRootCmd() + var stderr bytes.Buffer + root.SetOut(failingWriter{}) + root.SetErr(&stderr) + root.SetArgs(append(args, "--url", url)) + err := root.Execute() + if err == nil || exitCode(err) != exitInternal || !strings.Contains(err.Error(), "disk full") { + t.Errorf("err = %v (exit %d), want exit %d", err, exitCode(err), exitInternal) + } + }) + } + // JSON output kept its behaviour. + root := newRootCmd() + root.SetOut(failingWriter{}) + root.SetErr(&bytes.Buffer{}) + root.SetArgs([]string{"version", "--json"}) + if err := root.Execute(); err == nil || exitCode(err) != exitInternal { + t.Errorf("version --json: %v", err) + } +} + +// capture and template write a new file with mode 0600, never through a +// symbolic link and never half. They used to write mode 0644 (a capture +// holds serial numbers), follow a link to wherever it pointed, and truncate +// the old file before writing. +func TestHostileOutputFile(t *testing.T) { + dir := t.TempDir() + out := filepath.Join(dir, "device.json") + + if r := runCLI(t, "template", "--model", "1", "-o", out); r.err != nil { + t.Fatal(r.err) + } + if runtime.GOOS != "windows" { + if fi, err := os.Stat(out); err != nil || fi.Mode().Perm() != 0o600 { + t.Errorf("mode = %v, %v; want 0600", fi.Mode(), err) + } + } + // Replacing an existing, more open file gives the new mode too. + if err := os.Chmod(out, 0o644); err != nil { + t.Fatal(err) + } + if r := runCLI(t, "template", "--model", "1,103", "-o", out); r.err != nil { + t.Fatal(r.err) + } + df, err := devicefile.Load(out) + if err != nil || len(df.Models) != 2 { + t.Fatalf("rewritten file: %v", err) + } + if runtime.GOOS != "windows" { + if fi, _ := os.Stat(out); fi.Mode().Perm() != 0o600 { + t.Errorf("mode after replacing = %v", fi.Mode()) + } + } + if entries, _ := os.ReadDir(dir); len(entries) != 1 { + t.Errorf("temporary files left behind: %v", entries) + } + + // A symbolic link is refused, and what it names is left alone. + target := filepath.Join(dir, "target") + if err := os.WriteFile(target, []byte("keep"), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.json") + if err := os.Symlink(target, link); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + r := runCLI(t, "template", "--model", "1", "-o", link) + if r.err == nil || exitCode(r.err) != exitInternal || !strings.Contains(r.err.Error(), "symbolic link") { + t.Errorf("symlink: %v", r.err) + } + if data, _ := os.ReadFile(target); string(data) != "keep" { + t.Errorf("the link target was written: %q", data) + } + if fi, err := os.Lstat(link); err != nil || fi.Mode()&fs.ModeSymlink == 0 { + t.Errorf("the link was replaced: %v %v", fi, err) + } + + // A dangling link too: it would create the file it names. + dangling := filepath.Join(dir, "dangling.json") + if err := os.Symlink(filepath.Join(dir, "created-elsewhere"), dangling); err != nil { + t.Fatal(err) + } + if r := runCLI(t, "template", "--model", "1", "-o", dangling); r.err == nil { + t.Error("a dangling link was written through") + } + if _, err := os.Stat(filepath.Join(dir, "created-elsewhere")); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("file created through a dangling link: %v", err) + } + + // A directory, or a device such as /dev/null, is not a file to replace. + r = runCLI(t, "template", "--model", "1", "-o", dir) + if r.err == nil || !strings.Contains(r.err.Error(), "not a regular file") { + t.Errorf("directory: %v", r.err) + } + if runtime.GOOS != "windows" { + r = runCLI(t, "template", "--model", "1", "-o", os.DevNull) + if r.err == nil || !strings.Contains(r.err.Error(), "not a regular file") { + t.Errorf("%s: %v", os.DevNull, r.err) + } + } +} + +// When the new file cannot be written, the old one is kept whole. +func TestHostileOutputFileKeptOnFailure(t *testing.T) { + if runtime.GOOS == "windows" || os.Geteuid() == 0 { + t.Skip("needs a directory that refuses new files") + } + dir := t.TempDir() + out := filepath.Join(dir, "device.json") + if err := os.WriteFile(out, []byte("old"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chmod(dir, 0o500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + r := runCLI(t, "template", "--model", "1", "-o", out) + if r.err == nil || exitCode(r.err) != exitInternal { + t.Errorf("err = %v", r.err) + } + if data, _ := os.ReadFile(out); string(data) != "old" { + t.Errorf("old file = %q", data) + } +} + +func TestWriteFileAtomicErrors(t *testing.T) { + dir := t.TempDir() + // Lstat fails for a reason other than a missing file. + file := filepath.Join(dir, "f") + if err := os.WriteFile(file, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := writeFileAtomic(filepath.Join(file, "below"), []byte("x")); err == nil { + t.Error("a path below a file was written") + } + if err := writeFileAtomic(filepath.Join(dir, "missing", "x"), []byte("x")); err == nil { + t.Error("a path in a missing directory was written") + } +} + +// Files the commands read are bounded: a wrong path such as /dev/zero, or a +// huge file, is an error and not a process that eats all memory. +func TestHostileHugeInputFiles(t *testing.T) { + huge := filepath.Join(t.TempDir(), "huge.json") + f, err := os.Create(huge) + if err != nil { + t.Fatal(err) + } + if err := f.Truncate(maxDeviceFileSize + 1); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + paths := []string{huge} + if runtime.GOOS != "windows" { + if _, err := os.Stat("/dev/zero"); err == nil { + paths = append(paths, "/dev/zero") + } + } + for _, p := range paths { + r := runCLI(t, "serve", "--file", p, "--check") + if r.err == nil || exitCode(r.err) != exitInvalidInput || !strings.Contains(r.err.Error(), "file too large") { + t.Errorf("serve --file %s: %v", p, r.err) + } + ca := newTestCA(t, "CA") + cert, key := ca.issue("client") + for _, flags := range [][]string{ + {"--tls-cert", p, "--tls-key", key, "--tls-ca", ca.file}, + {"--tls-cert", cert, "--tls-key", p, "--tls-ca", ca.file}, + {"--tls-cert", cert, "--tls-key", key, "--tls-ca", p}, + } { + r := runCLI(t, append([]string{"models", "--url", "tcp+tls://127.0.0.1:1"}, flags...)...) + if r.err == nil || exitCode(r.err) != exitInvalidInput || !strings.Contains(r.err.Error(), "file too large") { + t.Errorf("%v: %v", flags, r.err) + } + } + } + // A directory cannot be read as a file. + if _, err := readFileLimited(t.TempDir(), 10); err == nil { + t.Error("a directory was read") + } + // The limit itself is allowed. + small := writeFile(t, "0123456789") + if data, err := readFileLimited(small, 10); err != nil || string(data) != "0123456789" { + t.Errorf("at the limit: %q, %v", data, err) + } + if _, err := readFileLimited(small, 9); !errors.Is(err, errTooLarge) { + t.Errorf("over the limit: %v", err) + } + // A CA file without certificates keeps its message. + ca := newTestCA(t, "CA") + cert, key := ca.issue("client") + r := runCLI(t, "models", "--url", "tcp+tls://127.0.0.1:1", "--tls-cert", cert, "--tls-key", key, "--tls-ca", writeFile(t, "")) + if r.err == nil || !strings.HasSuffix(r.err.Error(), ": no certificate found") { + t.Errorf("empty CA file: %v", r.err) + } +} + +// A failing last step (the rename) removes the temporary file and keeps the +// old one; a file that does not encode is not written at all. +func TestWriteDeviceFileFailures(t *testing.T) { + dir := t.TempDir() + out := filepath.Join(dir, "device.json") + if err := os.WriteFile(out, []byte("old"), 0o600); err != nil { + t.Fatal(err) + } + file := devicefile.New(devicefile.DefaultUnitID, devicefile.DefaultBaseAddress) + + saved := rename + rename = func(string, string) error { return errors.New("rename refused") } + err := writeDeviceFile(nil, out, file) + rename = saved + if err == nil || err.Error() != "rename refused" { + t.Errorf("err = %v", err) + } + if data, _ := os.ReadFile(out); string(data) != "old" { + t.Errorf("old file = %q", data) + } + if entries, _ := os.ReadDir(dir); len(entries) != 1 { + t.Errorf("temporary file left behind: %v", entries) + } + + id := uint16(1) + file.Models = append(file.Models, devicefile.Model{ID: &id, Group: devicefile.Group{Points: devicefile.Points{{Name: "Mn", Value: func() {}}}}}) + if err := writeDeviceFile(nil, out, file); err == nil { + t.Error("an unencodable file was written") + } + if data, _ := os.ReadFile(out); string(data) != "old" { + t.Errorf("old file = %q", data) + } +} + +func TestTermSafe(t *testing.T) { + for in, want := range map[string]string{ + "": "", + "Acme PV-5000 ü": "Acme PV-5000 ü", + "\x00\x7f": `\x00\x7f`, + "a\U000E0001b": `a\U000e0001b`, + "\u00a0": `\u00a0`, + `C:\dir`: `C:\dir`, + } { + if got := termSafe(in); got != want { + t.Errorf("termSafe(%q) = %q, want %q", in, got, want) + } + } + if err := safeError(nil); err != nil { + t.Errorf("safeError(nil) = %v", err) + } + plain := errors.New("plain") + if safeError(plain) != plain { + t.Error("an error without control characters was wrapped") + } +} + +// serve stops listening when it cannot print the layout as text, as it did +// for JSON. +func TestServeTextStatusWriteError(t *testing.T) { + root := newRootCmd() + root.SetOut(failingWriter{}) + root.SetErr(&bytes.Buffer{}) + url := "tcp://127.0.0.1:" + strconv.Itoa(freePort(t)) + root.SetArgs([]string{"serve", "--file", writeFile(t, exampleDevice), "--url", url}) + if err := root.Execute(); err == nil || exitCode(err) != exitInternal || !strings.Contains(err.Error(), "disk full") { + t.Errorf("err = %v", err) + } + if r := runCLI(t, "detect", "--url", url, "--timeout", "1s"); r.err == nil { + t.Error("the device is still listening") + } +} diff --git a/main.go b/main.go index 21660a1..ba2675f 100644 --- a/main.go +++ b/main.go @@ -9,6 +9,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "io" "os" @@ -95,6 +96,13 @@ func newRootCmd() *cobra.Command { root.AddCommand(detectCmd(), modelsCmd(), readCmd(), readModelCmd(), readPointCmd(), pollCmd(), pollModelCmd(), pollPointCmd(), serveCmd(), captureCmd(), templateCmd(), completionCmd(root), versionCmd()) + // Errors can quote what a device or a file holds: escape what a terminal + // would act on before cobra prints them. + for _, c := range root.Commands() { + if run := c.RunE; run != nil { + c.RunE = func(cmd *cobra.Command, args []string) error { return safeError(run(cmd, args)) } + } + } return root } @@ -172,6 +180,14 @@ func clientConfig() (modbus.Config, error) { if flagStopBits > 2 { return conf, withExit(exitUsage, fmt.Errorf("invalid --stop-bits %d: use 1 or 2", flagStopBits)) } + if flagTimeout <= 0 { + return conf, withExit(exitUsage, fmt.Errorf("invalid --timeout %s: use a positive duration such as 10s", flagTimeout)) + } + if flagUnitID == 0 { + // go-sunspec reads unit 1 for unit ID 0: refuse rather than read + // another unit than the one asked for. + return conf, withExit(exitUsage, errors.New("invalid --unit-id 0: that is the Modbus broadcast address, which a device does not answer; use 1-255")) + } cert, pool, err := loadTLS(flagURL) if err != nil { return conf, err @@ -209,8 +225,8 @@ func versionCmd() *cobra.Command { return &cobra.Command{ Use: "version", Short: "Print the sunspecctl version", - Run: func(cmd *cobra.Command, args []string) { - out := cmd.OutOrStdout() + RunE: func(cmd *cobra.Command, args []string) error { + out := newOutputWriter(cmd.OutOrStdout()) if flagJSON { info := map[string]string{"version": version} if tag != "" { @@ -222,8 +238,7 @@ func versionCmd() *cobra.Command { if buildDate != "" { info["buildDate"] = buildDate } - _ = printJSON(out, info) - return + return printJSON(out, info) } _, _ = fmt.Fprintf(out, "sunspecctl %s\n", version) if tag != "" { @@ -235,6 +250,7 @@ func versionCmd() *cobra.Command { if buildDate != "" { _, _ = fmt.Fprintf(out, "built: %s\n", buildDate) } + return out.result() }, } } @@ -265,14 +281,14 @@ func detectCmd() *cobra.Command { return printJSON(cmd.OutOrStdout(), result) } - out := cmd.OutOrStdout() + out := newOutputWriter(cmd.OutOrStdout()) _, _ = fmt.Fprintf(out, "Detected: %v\n", result.Detected) _, _ = fmt.Fprintf(out, "Unit ID: %d\n", result.UnitID) _, _ = fmt.Fprintf(out, "Base Address: %d\n", result.BaseAddress) _, _ = fmt.Fprintf(out, "Reg Type: %d\n", result.RegType) _, _ = fmt.Fprintf(out, "Marker: 0x%04X 0x%04X\n", result.Marker[0], result.Marker[1]) _, _ = fmt.Fprintf(out, "Attempts: %d\n", len(result.Attempts)) - return nil + return out.result() }, } } @@ -303,7 +319,8 @@ func modelsCmd() *cobra.Command { return printJSON(cmd.OutOrStdout(), device.Discovery) } - w := tabwriter.NewWriter(cmd.OutOrStdout(), 0, 4, 2, ' ', 0) + out := newOutputWriter(cmd.OutOrStdout()) + w := tabwriter.NewWriter(out, 0, 4, 2, ' ', 0) _, _ = fmt.Fprintln(w, "ID\tNAME\tSTART\tLENGTH\tSCHEMA") for _, m := range device.Discovery.Models { schema := "yes" @@ -311,10 +328,10 @@ func modelsCmd() *cobra.Command { schema = "no" } _, _ = fmt.Fprintf(w, "%d\t%s\t%d\t%d\t%s\n", - m.Header.ID, m.Name, m.Header.StartAddress, m.Header.Length, schema) + m.Header.ID, termSafe(m.Name), m.Header.StartAddress, m.Header.Length, schema) } _ = w.Flush() - return nil + return out.result() }, } } @@ -343,17 +360,18 @@ func readCmd() *cobra.Command { results, err := device.ReadAll(ctx) if err != nil { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: partial read: %v\n", err) + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: partial read: %s\n", termSafe(err.Error())) } if flagJSON { return printJSON(cmd.OutOrStdout(), results) } + out := newOutputWriter(cmd.OutOrStdout()) for _, dm := range results { - printDecodedModel(cmd.OutOrStdout(), dm) + printDecodedModel(out, dm) } - return nil + return out.result() }, } } @@ -391,8 +409,9 @@ func readModelCmd() *cobra.Command { return printJSON(cmd.OutOrStdout(), dm) } - printDecodedModel(cmd.OutOrStdout(), dm) - return nil + out := newOutputWriter(cmd.OutOrStdout()) + printDecodedModel(out, dm) + return out.result() }, } @@ -442,8 +461,9 @@ func readPointCmd() *cobra.Command { return printJSON(cmd.OutOrStdout(), dp) } - printPoint(cmd.OutOrStdout(), dp) - return nil + out := newOutputWriter(cmd.OutOrStdout()) + printPoint(out, dp) + return out.result() }, } @@ -466,17 +486,17 @@ func printJSON(out io.Writer, v any) error { // printPoint writes a single point as "Key: value" lines. With --raw it also // prints the point's register offset and count. func printPoint(out io.Writer, dp *sunspec.DecodedPoint) { - _, _ = fmt.Fprintf(out, "Point: %s\n", dp.Name) - _, _ = fmt.Fprintf(out, "Type: %s\n", dp.Type) - _, _ = fmt.Fprintf(out, "Raw: %v\n", dp.RawValue) + _, _ = fmt.Fprintf(out, "Point: %s\n", termSafe(dp.Name)) + _, _ = fmt.Fprintf(out, "Type: %s\n", termSafe(dp.Type)) + _, _ = fmt.Fprintf(out, "Raw: %s\n", termSafe(fmt.Sprint(dp.RawValue))) if dp.ScaledValue != nil { _, _ = fmt.Fprintf(out, "Scaled: %g\n", *dp.ScaledValue) } if dp.Units != "" { - _, _ = fmt.Fprintf(out, "Units: %s\n", dp.Units) + _, _ = fmt.Fprintf(out, "Units: %s\n", termSafe(dp.Units)) } if len(dp.Symbols) > 0 { - _, _ = fmt.Fprintf(out, "Symbols: %s\n", strings.Join(dp.Symbols, ", ")) + _, _ = fmt.Fprintf(out, "Symbols: %s\n", termSafe(strings.Join(dp.Symbols, ", "))) } if flagRaw { _, _ = fmt.Fprintf(out, "Offset: %d\n", dp.RegisterOffset) @@ -489,7 +509,7 @@ func printPoint(out io.Writer, dp *sunspec.DecodedPoint) { // indented under the group that contains them. With --raw it also dumps the // model's raw registers in hex. func printDecodedModel(out io.Writer, dm *sunspec.DecodedModel) { - _, _ = fmt.Fprintf(out, "=== Model %d: %s ===\n", dm.ModelID, dm.Name) + _, _ = fmt.Fprintf(out, "=== Model %d: %s ===\n", dm.ModelID, termSafe(dm.Name)) if dm.Group != nil { printGroup(out, "Fixed", dm.Group, " ") @@ -507,7 +527,7 @@ func printDecodedModel(out io.Writer, dm *sunspec.DecodedModel) { } for _, w := range dm.Warnings { - _, _ = fmt.Fprintf(out, " WARNING: %s\n", w) + _, _ = fmt.Fprintf(out, " WARNING: %s\n", termSafe(w)) } _, _ = fmt.Fprintln(out) } @@ -517,13 +537,13 @@ func printDecodedModel(out io.Writer, dm *sunspec.DecodedModel) { // then the instances nested inside it, labelled "name index" and indented one // level deeper. func printGroup(out io.Writer, label string, g *sunspec.DecodedGroup, indent string) { - _, _ = fmt.Fprintf(out, "%s[%s]\n", indent, label) + _, _ = fmt.Fprintf(out, "%s[%s]\n", indent, termSafe(label)) w := tabwriter.NewWriter(out, 0, 4, 2, ' ', 0) for _, p := range g.Points { if !p.Implemented { continue } - val := fmt.Sprintf("%v", p.RawValue) + val := fmt.Sprint(p.RawValue) if p.ScaledValue != nil { val = fmt.Sprintf("%g", *p.ScaledValue) } @@ -534,7 +554,7 @@ func printGroup(out io.Writer, label string, g *sunspec.DecodedGroup, indent str if len(p.Symbols) > 0 { extra += " [" + strings.Join(p.Symbols, ", ") + "]" } - _, _ = fmt.Fprintf(w, "%s %s\t%s\t%s%s\n", indent, p.Name, p.Type, val, extra) + _, _ = fmt.Fprintf(w, "%s %s\t%s\t%s\n", indent, termSafe(p.Name), termSafe(p.Type), termSafe(val+extra)) } _ = w.Flush() for _, c := range g.Groups { diff --git a/output.go b/output.go new file mode 100644 index 0000000..b5b6695 --- /dev/null +++ b/output.go @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: MIT + +package main + +import ( + "fmt" + "io" + "strings" + "unicode" + "unicode/utf8" +) + +// termSafe returns s with everything a terminal could act on escaped: control +// characters (C0, DEL, C1), Unicode format characters such as bidirectional +// overrides, any other non-printable rune, and bytes that are not UTF-8. +// Device strings, and errors that quote them, pass through it before they +// are printed as text; JSON output escapes on its own and keeps the original. +// +// Tab, newline and carriage return become \t, \n and \r, other ASCII +// controls and invalid bytes \xNN, other runes \uNNNN or \UNNNNNNNN. A +// string of printable characters is returned as it is. +func termSafe(s string) string { + i := 0 + for i < len(s) { + r, size := utf8.DecodeRuneInString(s[i:]) + if !printable(r, size) { + break + } + i += size + } + if i == len(s) { + return s + } + var b strings.Builder + b.Grow(len(s) + 16) + b.WriteString(s[:i]) + for i < len(s) { + r, size := utf8.DecodeRuneInString(s[i:]) + switch { + case printable(r, size): + b.WriteString(s[i : i+size]) + case r == utf8.RuneError && size == 1: + _, _ = fmt.Fprintf(&b, `\x%02x`, s[i]) + case r == '\t': + b.WriteString(`\t`) + case r == '\n': + b.WriteString(`\n`) + case r == '\r': + b.WriteString(`\r`) + case r < 0x80: + _, _ = fmt.Fprintf(&b, `\x%02x`, r) + case r <= 0xFFFF: + _, _ = fmt.Fprintf(&b, `\u%04x`, r) + default: + _, _ = fmt.Fprintf(&b, `\U%08x`, r) + } + i += size + } + return b.String() +} + +// printable reports whether the rune decoded with the given size can be +// printed as it is. +func printable(r rune, size int) bool { + if r == utf8.RuneError && size == 1 { + return false + } + return r == ' ' || unicode.IsPrint(r) +} + +// termSafeError is an error whose message is printed through termSafe. It +// unwraps to the original error, so the exit code does not change. +type termSafeError struct{ err error } + +func (e *termSafeError) Error() string { return termSafe(e.err.Error()) } +func (e *termSafeError) Unwrap() error { return e.err } + +// safeError wraps err so that its message is terminal safe. It returns err +// itself when nothing needs escaping, and nil for nil. +func safeError(err error) error { + if err == nil { + return nil + } + if msg := err.Error(); termSafe(msg) == msg { + return err + } + return &termSafeError{err: err} +} + +// outputWriter remembers the first error of the writer it wraps and refuses +// every write after it, so that text output, which is printed piecewise with +// the errors ignored, can still end the command with exit 10 when standard +// output fails (a full disk, a closed file). +type outputWriter struct { + w io.Writer + err error +} + +// newOutputWriter wraps w. +func newOutputWriter(w io.Writer) *outputWriter { return &outputWriter{w: w} } + +func (o *outputWriter) Write(p []byte) (int, error) { + if o.err != nil { + return 0, o.err + } + n, err := o.w.Write(p) + if err != nil { + o.err = err + } + return n, err +} + +// result returns nil, or the first write error as an internal error. +func (o *outputWriter) result() error { + if o.err == nil { + return nil + } + return withExit(exitInternal, fmt.Errorf("write output: %w", o.err)) +} diff --git a/poll.go b/poll.go index 7dadcda..b137c7d 100644 --- a/poll.go +++ b/poll.go @@ -46,6 +46,19 @@ func pollLoop(ctx context.Context, interval time.Duration, count int, fn func(ct return nil } +// checkPollFlags rejects an --interval or --count that cannot be meant: a +// zero or negative interval would poll the device as fast as it answers, and +// a negative count would end without a single poll. +func checkPollFlags(interval time.Duration, count int) error { + if interval <= 0 { + return withExit(exitUsage, fmt.Errorf("invalid --interval %s: use a positive duration such as 5s", interval)) + } + if count < 0 { + return withExit(exitUsage, fmt.Errorf("invalid --count %d: use 0 (poll until interrupted) or more", count)) + } + return nil +} + // pollCmd returns the command that repeatedly reads and prints every model. func pollCmd() *cobra.Command { var ( @@ -57,24 +70,30 @@ func pollCmd() *cobra.Command { Use: "poll", Short: "Repeatedly read and decode all models", RunE: func(cmd *cobra.Command, args []string) error { + if err := checkPollFlags(interval, count); err != nil { + return err + } client, cleanup, err := newClient() if err != nil { return err } defer cleanup() - ctx, cancel := context.WithTimeout(context.Background(), flagTimeout) + ctx, cancel := context.WithTimeout(cmd.Context(), flagTimeout) device, err := sunspec.Discover(ctx, client, discoverOpts()) cancel() if err != nil { + if cmd.Context().Err() != nil { + return nil // interrupted: a poll ends without error, see pollLoop + } return fmt.Errorf("discover: %w", err) } - out := cmd.OutOrStdout() + out := newOutputWriter(cmd.OutOrStdout()) return pollLoop(cmd.Context(), interval, count, func(ctx context.Context, iteration int) error { results, err := device.ReadAll(ctx) if err != nil { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: partial read: %v\n", err) + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: partial read: %s\n", termSafe(err.Error())) } if flagJSON { @@ -85,7 +104,7 @@ func pollCmd() *cobra.Command { for _, dm := range results { printDecodedModel(out, dm) } - return nil + return out.result() }) }, } @@ -107,20 +126,26 @@ func pollModelCmd() *cobra.Command { Use: "poll-model", Short: "Repeatedly read and decode a specific model by ID", RunE: func(cmd *cobra.Command, args []string) error { + if err := checkPollFlags(interval, count); err != nil { + return err + } client, cleanup, err := newClient() if err != nil { return err } defer cleanup() - ctx, cancel := context.WithTimeout(context.Background(), flagTimeout) + ctx, cancel := context.WithTimeout(cmd.Context(), flagTimeout) device, err := sunspec.Discover(ctx, client, discoverOpts()) cancel() if err != nil { + if cmd.Context().Err() != nil { + return nil // interrupted: a poll ends without error, see pollLoop + } return fmt.Errorf("discover: %w", err) } - out := cmd.OutOrStdout() + out := newOutputWriter(cmd.OutOrStdout()) return pollLoop(cmd.Context(), interval, count, func(ctx context.Context, iteration int) error { dm, err := device.ReadModelByID(ctx, modelID) if err != nil { @@ -133,7 +158,7 @@ func pollModelCmd() *cobra.Command { _, _ = fmt.Fprintf(out, "--- poll %d @ %s ---\n", iteration, time.Now().Format(time.RFC3339)) printDecodedModel(out, dm) - return nil + return out.result() }) }, } @@ -158,16 +183,22 @@ func pollPointCmd() *cobra.Command { Use: "poll-point", Short: "Repeatedly read a single named point from a model", RunE: func(cmd *cobra.Command, args []string) error { + if err := checkPollFlags(interval, count); err != nil { + return err + } client, cleanup, err := newClient() if err != nil { return err } defer cleanup() - ctx, cancel := context.WithTimeout(context.Background(), flagTimeout) + ctx, cancel := context.WithTimeout(cmd.Context(), flagTimeout) device, err := sunspec.Discover(ctx, client, discoverOpts()) cancel() if err != nil { + if cmd.Context().Err() != nil { + return nil // interrupted: a poll ends without error, see pollLoop + } return fmt.Errorf("discover: %w", err) } @@ -176,7 +207,7 @@ func pollPointCmd() *cobra.Command { return withExit(exitUnsupported, fmt.Errorf("model %d not found", modelID)) } - out := cmd.OutOrStdout() + out := newOutputWriter(cmd.OutOrStdout()) return pollLoop(cmd.Context(), interval, count, func(ctx context.Context, iteration int) error { dp, err := device.ReadPoint(ctx, *inst, pointName) if err != nil { @@ -189,7 +220,7 @@ func pollPointCmd() *cobra.Command { _, _ = fmt.Fprintf(out, "--- poll %d @ %s ---\n", iteration, time.Now().Format(time.RFC3339)) printPoint(out, dp) - return nil + return out.result() }) }, } diff --git a/serve.go b/serve.go index 8e73594..1d473ae 100644 --- a/serve.go +++ b/serve.go @@ -130,12 +130,26 @@ func writeLogger(out io.Writer) func(*sunspec.ServerWrite) { if p.Implemented && len(p.Symbols) > 0 { val += " [" + strings.Join(p.Symbols, ", ") + "]" } - parts = append(parts, p.Name+" = "+val) + parts = append(parts, termSafe(p.Name+" = "+val)) } - _, _ = fmt.Fprintf(out, "write from %s: model %d at %d: %s\n", w.ClientAddr, w.ModelID, w.Address, strings.Join(parts, ", ")) + _, _ = fmt.Fprintf(out, "write from %s: model %d at %d: %s\n", termSafe(w.ClientAddr), w.ModelID, w.Address, strings.Join(parts, ", ")) } } +// loadDeviceFile reads and parses a device file as devicefile.Load does, but +// reads at most maxDeviceFileSize bytes. +func loadDeviceFile(path string) (*devicefile.File, error) { + data, err := readFileLimited(path, maxDeviceFileSize) + if err != nil { + return nil, err + } + df, err := devicefile.Parse(data) + if err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + return df, nil +} + // serveCmd returns the command that runs a SunSpec device from a file. func serveCmd() *cobra.Command { var ( @@ -164,7 +178,7 @@ SunSpec defines as writable. Accepted writes are printed. The device runs until it is interrupted (Ctrl-C) or terminated.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { - df, err := devicefile.Load(file) + df, err := loadDeviceFile(file) if err != nil { return withExit(exitInvalidInput, fmt.Errorf("device file: %w", err)) } @@ -191,8 +205,9 @@ until it is interrupted (Ctrl-C) or terminated.`, if flagJSON { return printJSON(out, device) } - printServedDevice(out, "Valid", device) - return nil + w := newOutputWriter(out) + printServedDevice(w, "Valid", device) + return w.result() } cert, clientCAs, err := loadTLS(flagURL) @@ -217,7 +232,12 @@ until it is interrupted (Ctrl-C) or terminated.`, return err } } else { - printServedDevice(out, "Serving", device) + w := newOutputWriter(out) + printServedDevice(w, "Serving", device) + if err := w.result(); err != nil { + _ = server.Stop() + return err + } } ctx, stop := signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) diff --git a/testdata/fuzz/FuzzTermSafe/seed-c1-bidi b/testdata/fuzz/FuzzTermSafe/seed-c1-bidi new file mode 100644 index 0000000..245e0cc --- /dev/null +++ b/testdata/fuzz/FuzzTermSafe/seed-c1-bidi @@ -0,0 +1,2 @@ +go test fuzz v1 +string("\xc2\x9b31m\u2066x\u2069\U0001f600") diff --git a/testdata/fuzz/FuzzTermSafe/seed-forged-line b/testdata/fuzz/FuzzTermSafe/seed-forged-line new file mode 100644 index 0000000..c691302 --- /dev/null +++ b/testdata/fuzz/FuzzTermSafe/seed-forged-line @@ -0,0 +1,2 @@ +go test fuzz v1 +string("\x1b[31mred\r\nwrite from 6.6.6.6: forged") diff --git a/testdata/fuzz/FuzzTextOutput/seed-common-escapes b/testdata/fuzz/FuzzTextOutput/seed-common-escapes new file mode 100644 index 0000000..fa68e45 --- /dev/null +++ b/testdata/fuzz/FuzzTextOutput/seed-common-escapes @@ -0,0 +1,3 @@ +go test fuzz v1 +uint16(1) +[]byte("\x00\x01\x00B\x1b]0;pwn\a\x1b[2Ja\tb\nFAKE\xff\xfe\xe2\x80\xae") diff --git a/tls.go b/tls.go index 353a313..2fb9240 100644 --- a/tls.go +++ b/tls.go @@ -7,8 +7,6 @@ import ( "crypto/x509" "fmt" "strings" - - "github.com/otfabric/go-modbus" ) // TLS settings, used with tcp+tls:// URLs only (Modbus/TCP Security, which is @@ -47,13 +45,27 @@ func loadTLS(url string) (*tls.Certificate, *x509.CertPool, error) { if len(missing) > 0 { return nil, nil, withExit(exitUsage, fmt.Errorf("a tcp+tls:// URL needs %s", strings.Join(missing, ", "))) } - cert, err := tls.LoadX509KeyPair(flagTLSCert, flagTLSKey) + // The files are read with a size limit, then parsed as + // tls.LoadX509KeyPair and modbus.LoadCertPool would. + certPEM, err := readFileLimited(flagTLSCert, maxPEMFileSize) + if err != nil { + return nil, nil, withExit(exitInvalidInput, fmt.Errorf("--tls-cert/--tls-key: %w", err)) + } + keyPEM, err := readFileLimited(flagTLSKey, maxPEMFileSize) if err != nil { return nil, nil, withExit(exitInvalidInput, fmt.Errorf("--tls-cert/--tls-key: %w", err)) } - pool, err := modbus.LoadCertPool(flagTLSCA) + cert, err := tls.X509KeyPair(certPEM, keyPEM) + if err != nil { + return nil, nil, withExit(exitInvalidInput, fmt.Errorf("--tls-cert/--tls-key: %w", err)) + } + caPEM, err := readFileLimited(flagTLSCA, maxPEMFileSize) if err != nil { return nil, nil, withExit(exitInvalidInput, fmt.Errorf("--tls-ca: %w", err)) } + pool := x509.NewCertPool() + if !pool.AppendCertsFromPEM(caPEM) { + return nil, nil, withExit(exitInvalidInput, fmt.Errorf("--tls-ca: %s: no certificate found", flagTLSCA)) + } return &cert, pool, nil } From 36c28b1c9d3bf0cf2dca199aba45614c8f7b7c17 Mon Sep 17 00:00:00 2001 From: bartvanbos Date: Thu, 5 Nov 2026 14:56:22 +0100 Subject: [PATCH 3/3] chore(deps): go-sunspec v0.9.0, go-modbus v1.11.1; v0.5.0 notes Out-of-range scale factors are no longer applied by go-sunspec; the hostile test now asserts the unscaled point. Co-Authored-By: Claude Opus 5.5 --- RELEASE.md | 25 +++++++++++++++++++++++++ go.mod | 4 ++-- go.sum | 8 ++++---- hostile_device_test.go | 17 ++++++++++------- 4 files changed, 41 insertions(+), 13 deletions(-) diff --git a/RELEASE.md b/RELEASE.md index 51ad1ec..d6db404 100644 --- a/RELEASE.md +++ b/RELEASE.md @@ -1,5 +1,30 @@ # sunspecctl Releases +## v0.5.0 + +**Date:** 2026-11-05 +**Previous release:** v0.4.4 + +### Security + +- Text output and error messages escape control characters, escape sequences, bidirectional overrides and invalid UTF-8 in device data (shown as `\x1b`, `\n`, `\u202e`, `\xff`), so a device can no longer clear the screen, set the window title or forge output lines. `serve`'s write log is escaped too. JSON output is unchanged. +- `capture -o` and `template -o` write the file with mode 0600 through a temporary file and a rename, so the old file is kept if writing fails, and refuse a symbolic link or a non-regular file such as a directory or `/dev/null` (exit 10). `-o /dev/stdout` no longer works: leave out `-o` instead. +- `serve --file` reads at most 16 MiB, and `--tls-cert`, `--tls-key` and `--tls-ca` at most 4 MiB each; a larger file is exit 3 "file too large". + +### Fixed + +- Ctrl-C or SIGTERM during discovery stops `poll`, `poll-model` and `poll-point` at once (exit 0), instead of after `--timeout`. +- Text output that cannot be written exits 10 instead of 0, and `poll*` stops instead of polling on. JSON output already did this. +- Usage errors (exit 2): a `--timeout` that is not positive (was exit 5), an `--interval` that is not positive (polled with no pause), a negative `--count` (exit 0 with no output) and `--unit-id 0` on client commands (silently read unit 1). +- The docs said `detect` on a device without the SunSpec marker prints `Detected: false` and exits 0; it exits 1 and prints no result, as before. + +### Changed + +- Go 1.26 or later is required to build from source (was 1.23). +- Built on go-sunspec v0.9.0 (scale factors outside -10..10 are not applied) and go-modbus v1.11.1 (serial, UDP and server fixes). + +--- + ## v0.4.4 **Date:** 2026-11-03 diff --git a/go.mod b/go.mod index cf17ad0..ce69f59 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,8 @@ module github.com/otfabric/sunspecctl go 1.26.0 require ( - github.com/otfabric/go-modbus v1.10.6 - github.com/otfabric/go-sunspec v0.8.5 + github.com/otfabric/go-modbus v1.11.1 + github.com/otfabric/go-sunspec v0.9.0 github.com/spf13/cobra v1.10.2 ) diff --git a/go.sum b/go.sum index 4d49363..5bf2488 100644 --- a/go.sum +++ b/go.sum @@ -1,12 +1,12 @@ github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= -github.com/otfabric/go-modbus v1.10.6 h1:+ioZ6zejF+s16peVoWmYVYBTMTengVaV09mqx4sMlgM= -github.com/otfabric/go-modbus v1.10.6/go.mod h1:LciyfLtd+4wKhQrDV5TjdrRH3NB60A+5d9bzTtGHQW4= +github.com/otfabric/go-modbus v1.11.1 h1:9a5sMUwcpN49VVfCJRSgW72QggcfF7KnqiWNcbmnzSk= +github.com/otfabric/go-modbus v1.11.1/go.mod h1:FcVASTIAl1MNwPVW8nCMMubztiMueKHIFeVyHm0BDeY= github.com/otfabric/go-serial v0.2.1 h1:PCOdnCDAHIHFaxTBNmisC/4SlD+NVd74d+EJPnXrCOw= github.com/otfabric/go-serial v0.2.1/go.mod h1:TqbYpWWg6W+IuCLs8YM2cndlMAEaTyaKyUX6Z08gtq8= -github.com/otfabric/go-sunspec v0.8.5 h1:TIg5Dv+iWReFCzUEIoZChLJTMUQ+MsaulVuz76PhfLo= -github.com/otfabric/go-sunspec v0.8.5/go.mod h1:ZCLzbxifXHMWtPVrbRllZgti4BKvlmwrv3JXi/gOcc8= +github.com/otfabric/go-sunspec v0.9.0 h1:hKsK7I6wzQpVwibrkBv+wXD6LJxcuvxflTDUHl0J690= +github.com/otfabric/go-sunspec v0.9.0/go.mod h1:WqX973xM4bij9cqFr9leeXCCkXpMD8LJAe7SSbBOKKg= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/santhosh-tekuri/jsonschema/v6 v6.0.3 h1:1EYB5IzjZawrrnELUi78f9fPu57HuXjmddZPjrls/28= github.com/santhosh-tekuri/jsonschema/v6 v6.0.3/go.mod h1:JXeL+ps8p7/KNMjDQk3TCwPpBy0wYklyWTfbkIzdIFU= diff --git a/hostile_device_test.go b/hostile_device_test.go index aadc68b..ce51822 100644 --- a/hostile_device_test.go +++ b/hostile_device_test.go @@ -131,8 +131,9 @@ func jsonDocuments(t *testing.T, s string) int { } } -// A scale factor far outside SunSpec's -10..10 overflows the scaled value to -// infinity, or to NaN for a raw value of zero. +// A scale factor outside SunSpec's -10..10 is not applied (go-sunspec v0.9.0): +// the point keeps its raw value and has no scaled value, in JSON and in text. +// Before, it overflowed the scaled value to infinity, or to NaN for zero. func TestHostileScaleFactorOverflow(t *testing.T) { common := newModel(t, 1, 0).str("Mn", "Acme") inv := newModel(t, 101, 0). @@ -145,14 +146,16 @@ func TestHostileScaleFactorOverflow(t *testing.T) { var model map[string]any decodeJSON(t, r, &model) pts := jsonPoints(t, model) - if pts["W"]["ScaledValue"] != nil || pts["A"]["ScaledValue"] != nil || pts["Hz"]["ScaledValue"] != float64(0) { - t.Errorf("ScaledValue: W %#v, A %#v, Hz %#v", pts["W"]["ScaledValue"], pts["A"]["ScaledValue"], pts["Hz"]["ScaledValue"]) + for _, name := range []string{"W", "A", "Hz"} { + if pts[name]["ScaledValue"] != nil || pts[name]["SFRawValue"] != nil { + t.Errorf("%s: scale factor applied: %v", name, pts[name]) + } } - if pts["W"]["RawValue"] != float64(5) || pts["W"]["SFRawValue"] != float64(32767) { - t.Errorf("W: %v", pts["W"]) + if pts["W"]["RawValue"] != float64(5) || pts["Hz"]["RawValue"] != float64(5001) { + t.Errorf("raw values: W %v, Hz %v", pts["W"]["RawValue"], pts["Hz"]["RawValue"]) } r = runCLI(t, "read-point", "--model", "101", "--point", "W", "--url", url) - if r.err != nil || !strings.Contains(r.stdout, "Scaled: +Inf\n") { + if r.err != nil || !strings.Contains(r.stdout, "Raw: 5\n") || strings.Contains(r.stdout, "Inf") { t.Errorf("text: %v\n%s", r.err, r.stdout) } }