Error on malformed input when looking ahead about binser HOT 10 CLOSED

I agree that we should have bad input errors be distinguishable from bugs. I am working on this right now. The deserializer is pretty small so there are only a few asserts to add. Another class of error like this that I can imagine would be setting nil tables keys while deserializing.

from binser.

The lastest version 0.0-7 should address some (most?) of these kind of errors. We just assert every byte we read from input.

from binser.

Still some problems:

local binser = require "binser"

local error_patterns = {
   "Expected more bytes of input",
   "Could not deserialize type byte",
   "Expected more bytes of input",
   "Got nil resource name",
   "Expected table metatable"
}

local function test(...)
   local ok, err = pcall(binser.d, string.char(...))

   if ok then
      return
   end

   for _, error_pattern in ipairs(error_patterns) do
      if err:find(error_pattern) then
         return
      end
   end

   print("Bad error: ", err, ...)
end


test()

for c = 0, 255 do
   test(c)
end

-- This seems to get stuck in an infinite loop while allocating
-- Better disable swap before uncommenting
-- test(210, 25)

Bad error: binser.lua:149: attempt to compare nil with number on bytes:    206
Bad error: binser.lua:149: attempt to compare nil with number on bytes:    207
Bad error: binser.lua:149: attempt to compare nil with number on bytes:    208
Bad error: binser.lua:156: attempt to perform arithmetic on a nil value (local 'index') on bytes:  209
Bad error: binser.lua:149: attempt to compare nil with number on bytes:    210

from binser.

Also luacheck is angry at global newindex on line 385

from binser.

Thanks, I have added the above code to the test suite and fixed the bugs as well as the luacheck suggestion. I have also expanded the test to check all two byte combinations, although more would quickly get very slow. Perhaps some kind of fuzzer would be appropriate here.

from binser.

Some more cases found using random testing (https://gist.github.com/mpeterv/ecead7fcf9897aca3a027fbab7b50915):

\118\98\209\206\23

stack overflow
binser.lua:148: in upvalue 'number_from_str'
binser.lua:378: in upvalue 'deserialize_value'
binser.lua:412: in upvalue 'deserialize_value'
binser.lua:416: in upvalue 'deserialize_value
binser.lua:416: in function 'deserialize_value
binser.lua:416: in function 'deserialize_value

\206\203\126\5\176\72\30\208\109\253 (Lua 5.3 only)

bad argument #2 to 'byte' (number has no integer representation)
binser.lua:380: in upvalue 'deserialize_value'

\207\203\119\126\143\161\199\174\109\197

stuck in an infinite loop

\3\207\54\206\23

stack overflow

binser.lua:361: in function 'deserialize_value'
binser.lua:395: in function 'deserialize_value'
binser.lua:395: in function 'deserialize_value'
binser.lua:395: in function 'deserialize_value'

\207\140\149\188\132\1\210\19\227\172

stack overflow

binser.lua:427: in function 'deserialize_value'
binser.lua:401: in function 'deserialize_value'
binser.lua:400: in function 'deserialize_value'
binser.lua:400: in function 'deserialize_value'
binser.lua:400: in function 'deserialize_value

from binser.

Thanks again.

I think I have found the 2 main issues here. These test cases come from two main problems - not checking for negative length strings, and and not checking for termination in deserialization loops (this wasn't needed before as a random error would quickly occur if an unexpected end of input was reached. After the initial changes this instead caused an infinite loop - oops).

I have added the (modified) random data checks to the test suite as well.

from binser.

Thanks for fixing these! Do you plan to add an option or another function that would return nil + error message instead of throwing?

from binser.

I’m not sure. The easiest thing to do would be to wrap all calls to deserialize in a pcall and check for a known pattern. And often handling bad data and an error in binser would be the same for a user of the library.

from binser.

I can see how doing this with returns internally would be tricky, too much ok, err = ...; if not ok then return nil, err everywhere. Now that malformed inputs don't cause normal lua errors in binser, using whatever error is caught as a warning works well. Thanks again! Looking forward to 0.0-8.

from binser.