Files
go-kv/.omo/plans/fix-h1-h2-record-validation.md
dailz 57e7525ddf fix: validate physical record length > 0 and type (H1+H2)
DecodePhysicalRecord only checked length upper bound, not lower bound
(length > 0 per design §3.2 line 389). It also didn't validate the
fragment type field (RecInvalid=0 and types > RecLast are corruption
indicators per design §3.2 line 366-372).

Corrupt data with length=0 could pass CRC (payload is empty, CRC only
covers length+type bytes) and inject empty records into the fragment
collector. Invalid type values would only be caught at the collector
level, wrapped as TailCorruptionError, rather than rejected at the
parser level.

Changes:
- wal/record.go: DecodePhysicalRecord now rejects length=0 and
  type ∉ {RecFull..RecLast} before payload copy and CRC check.
  Checks are ordered to reject invalid records ASAP.
- wal/record_test.go: 5 tests:
  - TestDecodePhysicalRecord_RejectZeroLength (H1)
  - TestDecodePhysicalRecord_RejectInvalidType (H2, type=0)
  - TestDecodePhysicalRecord_RejectUnknownType (H2, type>RecLast)
  - TestDecodePhysicalRecord_ValidRecordsUnaffected (regression for
    all 4 valid types)
  - TestParseBlockWrapsInvalidRecordAsTailCorruption (integration:
    ParseBlock wraps H1/H2 errors as TailCorruptionError)

Verified: all existing tests pass. Full suite green including
go test -race ./... .

Audit context: docs/audit-3.2.md H1+H2.
2026-06-18 13:59:04 +08:00

8.5 KiB
Raw Permalink Blame History

H1 + H2 修复方案:Physical Record 加 length + type 校验

TL;DR

目标DecodePhysicalRecord 加两个校验:length > 0H1)和 type ∈ {Full, First, Middle, Last}(H2)。防止损坏数据注入非法 record 绕过语义检查。

交付

  • 2 个 if 检查加到 DecodePhysicalRecord
  • 4 个测试(length=0、type=0、type=5、正常回归)
  • 单次 commit

预估工时~1 小时 风险:极低。两行 if + 测试


Context

Bug 摘要

wal/record.go:39-68 DecodePhysicalRecord

func DecodePhysicalRecord(data []byte) (rec *PhysicalRecord, consumed int, err error) {
    ...
    crc := binary.LittleEndian.Uint32(data[0:4])
    length := binary.LittleEndian.Uint16(data[4:6])
    recType := data[6]

    if int(length) > len(data)-PhysicalRecordHeaderSize {  // 只校验上界
        return nil, 0, errors.New("record: data too short for payload")
    }
    // ← H1: 缺 length > 0 检查
    // ← H2: 缺 type 合法性检查
    ...
}

设计依据

§3.2 line 389

Physical Record 的 length 必须 > 0

§3.2 line 366-372 fragment 类型表:

| 0 | Invalid | 非法值,用于损坏检测 | | 1-4 | Full/First/Middle/Last | 合法 |

后果

Bug 触发条件 后果
H1 length=0 recordCRC 可匹配空 payload 空 record 进入 fragment collector,被分类为 TailCorruptionError 而非硬错误
H2 type=0 或 type>4 record 物理 CRC 可通过,到 collector 才报错。ParseBlock 包装为 TailCorruptionError(截断),而非设计期望的损坏检测

C4 fix 后,TailCorruptionError 在非尾段会被转硬错误。但尾段仍然截断。按设计,这类 invalid record 应该被 parser 层直接拒绝。

已有的零头检测

ParseBlock 在调 DecodePhysicalRecord 之前检查 isAllZeros(data[pos:pos+7])。如果 header 7 bytes 全 0,当作 padding 处理。所以 DecodePhysicalRecord 只在非全零 header 时被调。H1/H2 的校验覆盖:

  • length=0 但 type≠0header 非全零)→ H1 拒绝
  • type=0 但 length≠0 或 crc≠0header 非全零)→ H2 拒绝
  • type=5+ → H2 拒绝

执行计划

Phase A:代码改动(10 分钟)

A.1 DecodePhysicalRecord 加两个检查

文件:wal/record.go

func DecodePhysicalRecord(data []byte) (rec *PhysicalRecord, consumed int, err error) {
    if len(data) < PhysicalRecordHeaderSize {
        return nil, 0, errors.New("record: data too short for header")
    }

    crc := binary.LittleEndian.Uint32(data[0:4])
    length := binary.LittleEndian.Uint16(data[4:6])
    recType := data[6]

    // H1: length must be > 0 per design §3.2 line 389.
    if length == 0 {
        return nil, 0, errors.New("record: length must be > 0")
    }
    // H2: type must be valid per design §3.2 line 366-372 (RecInvalid=0
    // is for corruption detection; valid types are RecFull..RecLast).
    if recType < RecFull || recType > RecLast {
        return nil, 0, fmt.Errorf("record: invalid type %d", recType)
    }

    if int(length) > len(data)-PhysicalRecordHeaderSize {
        return nil, 0, errors.New("record: data too short for payload")
    }
    ...后续不变
}

检查顺序H1 → H2 → length 上界 → payload 复制 → CRC。非法 record 尽早拒绝,不做无意义的 payload 复制和 CRC 计算。

必要注释:引用设计行号,防回归。

Phase B:测试(30 分钟)

新增到 wal/record_test.go

func TestDecodePhysicalRecord_RejectZeroLength(t *testing.T) {
    // Construct: [crc 4][length=0 2][type=Full 1] = 7 bytes
    // CRC covers length(0,0) + type(1) = data[4:7]
    buf := make([]byte, PhysicalRecordHeaderSize)
    binary.LittleEndian.PutUint16(buf[4:6], 0)
    buf[6] = RecFull
    crc := crc32.Checksum(buf[4:7], crc32cTable)
    binary.LittleEndian.PutUint32(buf[0:4], crc)

    _, _, err := DecodePhysicalRecord(buf)
    if err == nil {
        t.Fatal("expected error for length=0")
    }
    if !strings.Contains(err.Error(), "length") {
        t.Errorf("error should mention length, got: %v", err)
    }
}

func TestDecodePhysicalRecord_RejectInvalidType(t *testing.T) {
    // Construct: [crc 4][length=4 2][type=0 (Invalid) 1][payload 4]
    payload := []byte("test")
    buf := make([]byte, PhysicalRecordHeaderSize+len(payload))
    binary.LittleEndian.PutUint16(buf[4:6], uint16(len(payload)))
    buf[6] = RecInvalid // 0
    copy(buf[7:], payload)
    crc := crc32.Checksum(buf[4:], crc32cTable)
    binary.LittleEndian.PutUint32(buf[0:4], crc)

    _, _, err := DecodePhysicalRecord(buf)
    if err == nil {
        t.Fatal("expected error for type=Invalid(0)")
    }
    if !strings.Contains(err.Error(), "type") {
        t.Errorf("error should mention type, got: %v", err)
    }
}

func TestDecodePhysicalRecord_RejectUnknownType(t *testing.T) {
    // type=5 (beyond RecLast=4)
    payload := []byte("test")
    buf := make([]byte, PhysicalRecordHeaderSize+len(payload))
    binary.LittleEndian.PutUint16(buf[4:6], uint16(len(payload)))
    buf[6] = 5 // RecLast=4, so 5 is unknown
    copy(buf[7:], payload)
    crc := crc32.Checksum(buf[4:], crc32cTable)
    binary.LittleEndian.PutUint32(buf[0:4], crc)

    _, _, err := DecodePhysicalRecord(buf)
    if err == nil {
        t.Fatal("expected error for type=5")
    }
    if !strings.Contains(err.Error(), "type") {
        t.Errorf("error should mention type, got: %v", err)
    }
}

// Regression: valid records still decode correctly.
func TestDecodePhysicalRecord_ValidRecordsUnaffected(t *testing.T) {
    for _, recType := range []uint8{RecFull, RecFirst, RecMiddle, RecLast} {
        t.Run(fmt.Sprintf("type=%d", recType), func(t *testing.T) {
            payload := []byte("test-payload")
            encoded := EncodePhysicalRecord(recType, payload)
            rec, consumed, err := DecodePhysicalRecord(encoded)
            if err != nil {
                t.Fatalf("type %d: %v", recType, err)
            }
            if rec.Type != recType {
                t.Errorf("Type = %d, want %d", rec.Type, recType)
            }
            if consumed != len(encoded) {
                t.Errorf("consumed = %d, want %d", consumed, len(encoded))
            }
        })
    }
}

Phase C:验证(10 分钟)

# 1. 编译
go build ./...

# 2. 重点测试
go test ./wal -run 'TestDecodePhysicalRecord' -count=1 -v

# 3. wal 包全量
go test ./wal/... -count=1

# 4. 全仓
go test ./... -count=1

# 5. race
go test -race ./... -count=1

# 6. vet
go vet ./...

Phase DCommit message draft

fix: validate physical record length > 0 and type (H1+H2)

DecodePhysicalRecord only checked length upper bound, not lower bound
(length > 0 per design §3.2 line 389). It also didn't validate the
fragment type field (RecInvalid=0 and types > RecLast are corruption
indicators per design §3.2 line 366-372).

Corrupt data with length=0 could pass CRC (payload is empty, CRC only
covers length+type bytes) and inject empty records into the fragment
collector. Invalid type values would only be caught at the collector
level, wrapped as TailCorruptionError, rather than rejected at the
parser level.

Changes:
- wal/record.go: DecodePhysicalRecord now rejects length=0 and
  type ∉ {RecFull..RecLast} before payload copy and CRC check.
  Checks are ordered to reject invalid records ASAP.
- wal/record_test.go: 4 tests:
  - TestDecodePhysicalRecord_RejectZeroLength (H1)
  - TestDecodePhysicalRecord_RejectInvalidType (H2, type=0)
  - TestDecodePhysicalRecord_RejectUnknownType (H2, type=5)
  - TestDecodePhysicalRecord_ValidRecordsUnaffected (regression for
    all 4 valid types)

Verified: all existing tests pass. Full suite green including
go test -race ./... .

Audit context: docs/audit-3.2.md H1+H2.

验收清单

  • Phase A.1length == 0 检查存在
  • Phase A.1recType < RecFull || recType > RecLast 检查存在
  • Phase B4 个测试存在并 PASS
  • go test ./wal/... -count=1 全绿
  • go test -race ./... -count=1 全绿
  • go vet ./... 无新增警告
  • 单次 commitmessage 引用 audit H1+H2

不在本次范围内

为什么不放进来
H3 worstCaseSize 估算,独立
H4 MaxImmutableCount,独立
H5-H7 模型偏离,独立
ParseBlock 错误分类 当前所有 DecodePhysicalRecord 错误包装为 TailCorruptionError,由 C4 的 isLastSegment 区分。不改

修订记录

  • v1(原始):H1+H2 修复方案初稿,送 Momus 审