rossvideo / Catena

Other
4 stars 0 forks source link

problem: array types have no maximum size* #145

Open mejohnnaylor opened 3 months ago

mejohnnaylor commented 3 months ago

so, repeated calls to SetValue("/some/oid/or/other/-", value), which currently get interpreted as "append the value" could be abused to exhaust memory and DoS the service.

some options for dealing with this: 1) don't support append behavior 2) overload the max_length param property, currently only valid for String Types to use as a limit on the number of elements in an array. I don't know how this interferes with STRING_ARRAY 3) add a max_size, or capacity field for array types and have it mean the maximum # of elements allowed in the array. Of course protobuf would default this to 0, so maybe we need a "default default" for this - say 32, 64 or some other reasonable power of 2?

*STRING_ARRAY does, kinda, sorta - it has total_length which is the total length of all the strings in the array, not the max number of strings in the array which is what this issue is concerned with.

example usage for option 3:

"eq": {
  "type": "STRUCT_ARRAY",
  "name": {"en": "Four Band EQ"},
  "params": {
    "response": {...}
    "freq": {...},
    "gain": {...},
    "Q": {...}
  },
  "capacity": 4,
  "value": [ // N.B. simplified (i.e. incorrect) syntax
    {
      "response": "Low Shelf",
      "freq": 80.0
      "gain": -14.0
      "Q": 0.707
    },
   {
      "response": "Bell",
      "freq": 400.0
      "gain": 6.0
      "Q": 0.707
    },
   {
      "response": "Bell",
      "freq": 2500.0
      "gain": -8.9
      "Q": 0.707
    }
   ]
}

SetValue("/eq/-", {"response": "High Shelf", "freq": 7500.0, "gain": 10.0, "Q": 0.707}) // should succeed because capacity exists SetValue("/eq/-", {"response": "Notch", "freq": 50.0, "Q": 10.0}) // should fail because capacity eq already has 4 bands

N.B. there isn't any JSON pointer syntax for "remove element from arbitrary point in the array". A client could work around this by using the "SetAllValues" semantics (element_index = -1)

james-peltzer commented 3 months ago

I don't think the "-" suffix is necessary to support at this time since all of these pointers are to fully-qualified parameters. Historically, we always supply the index we are looking for anything we're setting. It is up to the server to determine whether (or how much) an array can grow.