Posted at 2023-02-01 by @gfwilliams

I'm not sure I understand - where in _jswrap_interface_setTimeoutOrInterval do you see a problem?

And where is the issue in jsvObjectSetChildAndUnLock and how can it get an overflow? Maybe you can point me to the bit of code that's a problem.

Realistically in Espruino arrays of JsVars are extremely rare and it's pretty easy to see that everything is in range, so I wouldn't bother with overflow checking.

What is important is checking your locks/unlocks all match but there's no real way to check that short of running your code and checking for memory leaks

Posted at 2023-02-01 by @fanoush

@!evil Just to make sure, by "overflow" you always mean "out of memory"?

Posted at 2023-02-01 by !evil

Sorry for the confusion, i meant "out of memory", not out of array bounds. I hope the following is better.

In _jswrap_interface_setTimeoutOrInterval, timerPtr is zero if there isn't enough memory. This might not have immediate consequences if all future ops align well (like jsvObjectSetChildAndUnLock). Or itemIndex may be zero despite a new timer. The consequences in this particular case may be negligible, so it may be just a bad example.

Regarding jsvObjectSetChildAndUnLock: if jsvObjectSetChildVar returns zero due to "out of memory", it won't unlock child (https://github.com/espruino/Espruino/blob/7277a8dc9377e09f9a6837ff754e3dd8dd761563/src/jsvar.c#L3005). Compare with jsvArrayPushAndUnLock where value is unlocked regardless (https://github.com/espruino/Espruino/blob/7277a8dc9377e09f9a6837ff754e3dd8dd761563/src/jsvar.c#L3298).

The main question is: care about "out of memory" (e.g. if (var) { ...; jsvUnLock(var); } all the way) because there is a sane future for the system after the incident, or don't and assume nothing bad will happen. Both viable imo.

Posted at 2023-02-01 by @gfwilliams

Ahh, right - that makes a lot more sense!

Thanks for that - in _jswrap_interface_setTimeoutOrInterval I think there isn't a huge issue because jsvObjectSetChild is null-safe, although the jsvObjectSetChildAndUnLock that it calls could still 'lose' some locks. I'll add a guard in anyway.

jsvObjectSetChildAndUnLock is a really good spot - thanks! I've just pushed a fix for that.

It's definitely good practice to check when allocating something like an object but I wouldn't worry about trying to clean up after yourself in the library. When you do 'SetChildAndUnLock(.... jsvNew...)` then memory errors really don't matter at all.

Espruino should continue working just fine after an out of memory error - and actually the Linux build does have a 'memtest' where it'll just run the tests time after time, slowly decreasing memory so that it can see if anything breaks badly when out of memory happens at different times.

If a lock is 'leaked' it's not the end of the world. It basically just means that var can't be garbage collected, so it might result in a memory leak - but in many cases it probably won't even be noticed.

Posted at 2023-02-01 by !evil

Espruino should continue working just fine after an out of memory
error - and actually the Linux build does have a 'memtest' where it'll
just run the tests time after time, slowly decreasing memory so that
it can see if anything breaks badly when out of memory happens at
different times.

This is very reassuring :) Thanks for the guidance!