Overflow checks

Hi,

I was wondering what the recommended approach to memory overflow is. Most of jsvar.c check for overflow and behave accordingly, while other code assume no overflow, e.g. _jswrap_interface_setTimeoutOrInterval. Then there is jsvObjectSetChildAndUnLock which doesn't unlock the child in case of overflow, while others like jsvArrayPushAndUnLock do.

I tried to use the following defines for perfect overflow coverage in my library

﹟define INIT(n) JsVar* _lock_array[n]; int _lock_index = 0
﹟define END(i) _end(i, _lock_array, _lock_index)
﹟define NEW(a) ({ JsVar* var = a; if (!var) return END(0); _lock_array[_lock_index++] = var; var; })
﹟define TMP(a) ({ JsVar* var = a; if (!var) return END(0); var; })
﹟define GRD(a) if (!(a)) return END(0);


JsVar* _end(int end_index, JsVar** array, int index) {
  while (index != end_index) jsvUnLock(array[--index]);
  return 0;
}

but I'm uncertain if its worth the trouble. What if I won't check for overflow, like _jswrap_interface_setTimeoutOrInterval, would it lead to unexpected behavior in case of overflow instead of noisy "segfault"?

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

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

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.

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.

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!