thomasokken / free42

Free42 : An HP-42S Calculator Simulator
https://thomasokken.com/free42/
GNU General Public License v2.0
287 stars 55 forks source link

malloc with zero size could return NULL #37

Closed rverpillot closed 2 years ago

rverpillot commented 2 years ago

After compiling and executing Free42 v3.0.9 on my DM42 calculator, I got some memory errors on NEWLIST and APPEND instructions.

So, I found you allocate memory with zero size when using NEWLIST. But malloc(0) could return NULL pointer with some libc libraries.

https://github.com/thomasokken/free42/blob/93e30e8277ed3fdb935b80ea5d89cbabe25a8ada/common/core_variables.cc#L175 https://github.com/thomasokken/free42/blob/93e30e8277ed3fdb935b80ea5d89cbabe25a8ada/common/core_variables.cc#L442

I currently just fixed it by checking if malloc return NULL pointer only when size > 0 .

thomasokken commented 2 years ago

Yes, there are lots of places where Free42 checks for malloc() failure by comparing its result to NULL. I've been using that idiom since forever, and didn't know until recently that malloc(0) may return NULL. On the platforms I support, with the tools I use, this doesn't appear to be an issue, malloc(0) does not return NULL on any of those. But if this is an issue on the DM42 or elsewhere, a quick and easy fix would be to replace malloc() with a wrapper that checks for malloc(0) and calls malloc(1) instead.

Or find all the places where Free42 can call malloc(0) and fix them. I briefly looked into this a couple of months ago and decided to leave it, since it's not a problem on the platforms I support...

thomasokken commented 2 years ago

On second thought, there can't be very many cases like that. Matrices are never size 0, and when strings are short, they are handled differently anyway. And you're right, the required code change should be simple in most cases, just replace "if (ptr == NULL)" with "if (ptr == NULL && size != 0)". And just to be safe, a review that having NULL in a valid object doesn't cause weirdness elsewhere.

It doesn't sound so bad after all. I'll take care of it, this weekend or next week probably.

rverpillot commented 2 years ago

The official DM42 firmware (based on 3.0.5) doesn't have workarounds for theses cases and it works flawlessly. With current Free42 version, I only found these 2 lines where the problem arises, because NEWLIST explicitly allocate size 0.

thomasokken commented 2 years ago

Fixed. I looked at all malloc() and realloc() calls in all of common/* and found only three that may be called with size 0. I added the extra check for all three. e033a384c96b62cce62ff8ac8b5e83ab1fa33d1d