search

oxidecomputer / omicron

Omicron: Oxide control plane
Mozilla Public License 2.0
241 stars 36 forks source link

CRDB silently truncates size limited strings in an array #6514

Open papertigers opened 1 week ago

papertigers commented 1 week ago

While working on #6275 I was writing a test to verify some of the constraints set by the following table:

CREATE TABLE IF NOT EXISTS omicron.public.inv_nvme_disk_firmware (
    -- where this observation came from
    -- (foreign key into `inv_collection` table)
    inv_collection_id UUID NOT NULL,

    -- unique id for this sled (should be foreign keys into `sled` table, though
    -- it's conceivable a sled will report an id that we don't know about)
    sled_id UUID NOT NULL,
    -- The slot where this disk was last observed
    slot INT8 CHECK (slot >= 0) NOT NULL,

    -- total number of firmware slots the device has
    number_of_slots INT2 CHECK (number_of_slots BETWEEN 1 AND 7) NOT NULL,
    active_slot INT2 CHECK (number_of_slots BETWEEN 1 AND 7) NOT NULL,
    -- staged firmware slot to be active on reset
    next_active_slot INT2 CHECK (number_of_slots BETWEEN 1 AND 7),
    -- slot1 is distinct in the NVMe spec in the sense that it can be read only
    slot1_is_read_only BOOLEAN,
    -- the firmware version string for each NVMe slot (0 indexed), a NULL means the
    -- slot exists but is empty
    slot_firmware_versions STRING(8)[] CHECK (array_length(slot_firmware_versions, 1) BETWEEN 1 AND 7),

    -- PK consisting of:
    -- - Which collection this was
    -- - The sled reporting the disk
    -- - The slot in which the disk was found
    PRIMARY KEY (inv_collection_id, sled_id, slot)
);

In particular I was writing a test that would attempt to input an NVMe firmware version string that was larger than a STRING(8) in the slot_firmware_versions ARRAY with a value of vec![Some("firmwaretoolarge")]. What I found however was a little odd as the test was failing on expect_err('XXX') as the test was able to insert data just fine. Using dev-db I was able to import the state of CRDB from the test and found that CRDB silently truncated the firmware version string to just FIRMWARE:

-[ RECORD 6 ]
inv_collection_id      | c84c2603-d30f-46a3-b696-ae122a825e00
sled_id                | 5df4ff26-28a7-4961-8b20-45a33358b56c
slot                   | 1
number_of_slots        | 1
active_slot            | 1
next_active_slot       | NULL
slot1_is_read_only     | true
slot_firmware_versions | {FIRMWARE}

The CRDB docs state the following about bounding a strings size:

When inserting a STRING value or a STRING-related-type value:

  • If the value is cast with a length limit (e.g., CAST('hello world' AS STRING(5))), CockroachDB truncates to the limit. This applies to STRING(n) and all related types.
  • If the value exceeds the column's length limit, CockroachDB returns an error. This applies to STRING(n) and all related types.
  • For STRING(n) and VARCHAR(n)/CHARACTER VARYING(n) types, if the value is under the column's length limit, CockroachDB does not add space padding to the end of the value.
  • For CHAR(n)/CHARACTER(n) types, if the value is under the column's length limit, CockroachDB adds space padding from the end of the value to the length limit.

My first thought was that somewhere along the way from rust -> diesel -> crdb we were somehow casting to the string in the array. To investigate this path I used diesel-dtrace and found that we are in fact not using CAST anywhere in the raw sql statement.

$ pfexec dtrace -x strsize=4096 -Zqn 'diesel*:::query-start {printf("%s\n\n\n", copyinstr(arg2))}'
..... snip ......
INSERT INTO "inv_nvme_disk_firmware" ("inv_collection_id", "sled_id", "slot", "active_slot", "next_active_slot", "number_of_slots", "slot1_is_read_only", "slot_firmware_versions") VALUES ($1, $2, $3, $4, DEFAULT, $5, $6, $7), ($8, $9, $10, $11, DEFAULT, $12, $13, $14), ($15, $16, $17, $18, DEFAULT, $19, $20, $21),
($22, $23, $24, $25, DEFAULT, $26, $27, $28), ($29, $30, $31, $32, DEFAULT, $33, $34, $35) -- binds: [d030bd8d-1049-45a4-a7b1-67e24303f186 (collection), 5c5b4cf9-3e13-45fd-871c-f177d6537510 (sled), 1, SqlU8(1), SqlU8(1), true, [Some("firmwareistoolong")], d030bd8d-1049-45a4-a7b1-67e24303f186 (collection), c5aec1df-
b897-49e4-8085-ccd975f9b529 (sled), 0, SqlU8(1), SqlU8(1), true, [Some("EXAMP1")], d030bd8d-1049-45a4-a7b1-67e24303f186 (collection), c5aec1df-b897-49e4-8085-ccd975f9b529 (sled), 1, SqlU8(1), SqlU8(1), true, [Some("EXAMP1")], d030bd8d-1049-45a4-a7b1-67e24303f186 (collection), c5aec1df-b897-49e4-8085-ccd975f9b529 (s
led), 2, SqlU8(1), SqlU8(1), true, [Some("EXAMP1")], d030bd8d-1049-45a4-a7b1-67e24303f186 (collection), c5aec1df-b897-49e4-8085-ccd975f9b529 (sled), 3, SqlU8(1), SqlU8(1), true, [Some("EXAMP1")]]
...... snip .....

Next I wanted to recreate the issue in the crdb repl on my Mac so I created the table and attempted to run an insert command again:

demo@127.0.0.1:26257/movr>
                        -> CREATE TABLE IF NOT EXISTS inv_nvme_disk_firmware (
                        -> -- where this observation came from
                        -> -- (foreign key into `inv_collection` table)
                        -> inv_collection_id UUID NOT NULL,
                        ->
                        -> -- unique id for this sled (should be foreign keys into `sled` table, though
                        -> -- it's conceivable a sled will report an id that we don't know about)
                        -> sled_id UUID NOT NULL,
                        -> -- The slot where this disk was last observed
                        -> slot INT8 CHECK (slot >= 0) NOT NULL,
                        ->
                        -> -- total number of firmware slots the device has
                        -> number_of_slots INT8 CHECK (number_of_slots BETWEEN 1 AND 7) NOT NULL,
                        -> active_slot INT8 CHECK (number_of_slots BETWEEN 1 AND 7) NOT NULL,
                        -> -- staged firmware slot to be active on reset
                        -> next_active_slot INT8 CHECK (number_of_slots BETWEEN 1 AND 7),
                        -> -- slot1 is distinct in the NVMe spec in the sense that it can be read only
                        -> slot1_is_read_only BOOLEAN,
                        -> -- the firmware version string for each NVMe slot (0 indexed), a NULL means the
                        -> -- slot exists but is empty
                        -> slot_fw_versions STRING(8)[],
                        -> CONSTRAINT check_slot_fw_versions_len CHECK (array_length(slot_fw_versions, 1) BETWEEN 1 AND 7),
                        ->
                        -> -- PK consisting of:
                        -> -- - Which collection this was
                        -> -- - The sled reporting the disk
                        -> PRIMARY KEY (inv_collection_id, sled_id, slot)
                        -> );
                        ->
CREATE TABLE

demo@127.0.0.1:26257/movr> insert into inv_nvme_disk_firmware (inv_collection_id, sled_id, slot, number_of_slots, active_slot, next_active_slot, slot1_is_read_only, slot_fw_versions) values ('C84C2603-D30F-46A3-B696-AE122A825E00', '5DF4FF26-28A7-4961-8B20-45A33358B56C', 1, 1, 1, NULL, true,
                        -> ARRAY['FIRMWARETOOLONG']);
ERROR: value too long for type STRING(8)
SQLSTATE: 22001

Oddly enough I received the error I was expecting to see in my unit test. My next thought was that the crdb version I have installed via brew on my Mac is newer than what we are using in the control plane, so I fired up dev-db again and attempted the same experiment:

root@localhost:32221/defaultdb> CREATE TABLE IF NOT EXISTS inv_nvme_disk_firmware (
    -- where this observation came from
    -- (foreign key into `inv_collection` table)
    inv_collection_id UUID NOT NULL,

    -- unique id for this sled (should be foreign keys into `sled` table, though
    -- it's conceivable a sled will report an id that we don't know about)
    sled_id UUID NOT NULL,
    -- The slot where this disk was last observed
    slot INT8 CHECK (slot >= 0) NOT NULL,

    -- total number of firmware slots the device has
    number_of_slots INT2 CHECK (number_of_slots BETWEEN 1 AND 7) NOT NULL,
    active_slot INT2 CHECK (number_of_slots BETWEEN 1 AND 7) NOT NULL,
    -- staged firmware slot to be active on reset
    next_active_slot INT2 CHECK (number_of_slots BETWEEN 1 AND 7),
    -- slot1 is distinct in the NVMe spec in the sense that it can be read only
    slot1_is_read_only BOOLEAN,
    -- the firmware version string for each NVMe slot (0 indexed), a NULL means the
    -- slot exists but is empty
    slot_firmware_versions STRING(8)[] CHECK (array_length(slot_firmware_versions, 1) BETWEEN 1 AND 7),

    -- PK consisting of:
    -- - Which collection this was
    -- - The sled reporting the disk
    -- - The slot in which the disk was found
    PRIMARY KEY (inv_collection_id, sled_id, slot)
);
CREATE TABLE

Time: 6ms total (execution 6ms / network 0ms)

root@localhost:32221/defaultdb>  insert into inv_nvme_disk_firmware (inv_collection_id, sled_id, slot, number_of_slots, active_slot, next_active_slot, slot1_is_read_only, slot_firmware_versions) values ('C84C2603-D30F-46A3-B696-AE122A825E00', '5DF4FF26-28A7-4961-8B20-45A33358B56C', 1, 1, 1, NULL, true, ARRAY['FIRMWARETOOLONG']);
INSERT 1

Time: 7ms total (execution 7ms / network 0ms)

It appears upstream has fixed this bug at some point along the way.

control plan crdb version:

cockroach version
Build Tag:        v22.1.9-dirty
Build Time:       2022/10/26 21:17:46
Distribution:     OSS
Platform:         illumos amd64 (x86_64-pc-solaris2.11)
Go Version:       go1.17.13
C Compiler:       gcc 10.3.0
Build Commit ID:  e438c2f89282e607e0e6ca1d38b2e0a622f94493
Build Type:       release

macOS crdb version:

❯ cockroach version
Build Tag:        v24.1.3
Build Time:       2024/08/01 11:50:06
Distribution:     CCL
Platform:         darwin arm64 (aarch64-apple-darwin21.2)
Go Version:       go1.22.5 X:nocoverageredesign
C Compiler:       Clang 10.0.0
Build Commit ID:  9e10212477fde97f55ea4fff01797288c836575c
Build Type:       release
Enabled Assertions: false
papertigers commented 1 week ago

Using https://github.com/oxidecomputer/omicron/commit/ce73bc179271eecdb0b048fd5620a748a9f0f952 to grab crdb version 22.2.19 we can see that the database no longer silently truncates the string:

demo@127.0.0.1:26257/movr> insert into inv_nvme_disk_firmware (inv_collection_id, sled_id, slot, number_of_slots, active_slot, next_active_slot, slot1_is_read_only, slot_firmware_versions) values ('C84C2603-D30F-46A3-B696-AE122A825E00', '5DF4FF26-28A7-4961-8B20-45A33358B56C', 1, 1, 1, NULL, true, ARRAY['FIRMWARETOOLONG']);
ERROR: value too long for type STRING(8)
SQLSTATE: 22001