cbor-x icon indicating copy to clipboard operation
cbor-x copied to clipboard

Added security checks to prevent memory overflow.

Open JimmyBjorklund opened this issue 2 years ago • 6 comments

When some one send bad data we can add some simple checks to prevent at least some of the easy crashes.

JimmyBjorklund avatar Mar 26 '24 10:03 JimmyBjorklund

JavaScript already has memory overflow checks in most cases, what is the purpose of setting different limits than JavaScipt's limits? Is there a specific error that this handles more gracefully? Why was 1,000,000 chosen (0xf4240 seems like kind of an odd limit for a computer)?

kriszyp avatar Mar 26 '24 14:03 kriszyp

The reson is that its not nice to have an application crash every time some sends faulty data. This prevents a system crash and produces a much better error that can be handled. The worst case i have bin able to cause is so that the library get stuck in an infinit loop. The built in security can not fix and this fixes both cases. The limit is just any number that is big enough to not cause any issues for normal users.

JimmyBjorklund avatar Mar 26 '24 15:03 JimmyBjorklund

Hmm, it seems like there are different errors for arrays and Maps, some of which are recoverable and some aren't. And this varies across different runtimes. This doesn't seem to take that into account.

The limit is just any number that is big enough

What if the user wants more than the limit, and that limit is legitimately within the runtime's capabilities?

kriszyp avatar Mar 26 '24 16:03 kriszyp

Added the ability to change the limit

JimmyBjorklund avatar Mar 26 '24 21:03 JimmyBjorklund

Its always better to have valid, error the letting the application crash or deadlock as it is now if invalid data is send in. @kriszyp any plans to merge this ?

JimmyBjorklund avatar Apr 12 '24 07:04 JimmyBjorklund

Yes, I would, but definitely still some work to be done, as the defaults haven't been aligned with the runtime's capabilities (they are completely arbitrary). And introduction of code like this should be benchmarked to determine the performance regression. There are also no unit tests or documentation in here either. I do plan to get to this, but it is definitely not ready, there are a number of remaining tasks, and I haven't had time to get to these.

kriszyp avatar Apr 12 '24 12:04 kriszyp