gphoto / libgphoto2

The libgphoto2 camera access and control library.
GNU Lesser General Public License v2.1
1.02k stars 324 forks source link

out-of-memory condition is handled widely inconsistently #1008

Open axxel opened 1 week ago

axxel commented 1 week ago

There are currently almost 1000 calls to memory allocating functions (malloc, calloc, realloc, strdup) all over the code base. There are 3 inconsistent levels of dealing with a failed allocation:

  1. don't check the result at all, this will most likely later lead to a SEGFAULT when accessing that pointer
  2. check for NULL and return an error (mostly GP_ERROR_NO_MEMORY or PTP_RC_GeneralError)
  3. check for 'NULL', properly free locally allocated/temporary memory, then return an error

Option 1) occurs the most by far, option 3) only very seldomly.

I would argue that option 3) is definitively overkill and merely introduces noise in the code base. Realistically, I'd argue that option 2) is also virtually useless in real-world scenarios. If your process ran out of memory, it will shut down either way, pretty soon. To my knowledge, on Linux, it might very well be killed by the OOM killer before you ever get a failed malloc.

My suggestion would be to either completely do away with verbose NULL checks for failed allocations (only use option 1), which will then result in SEGFAULTS. Or come up with something like xalloc that puts out some kind of "out of memory" error message and then aborts the program, which obviously provides more information than simply SEGFAULting.

Leaving the status quo is of course also an option... ;)

msmeissn commented 1 week ago

I do not remember ever getting a bugreport for this kind of thing.

I would not spend effort on it currently , only recommend defensive programming in future code.

axxel commented 1 week ago

I do not remember ever getting a bugreport for this kind of thing.

That just proves my point :). Having NULL checks for maybe 30% of all those (tiny) allocations is pointless because a) they never fail and b) if one would, the user still has a 70% chance of crashing his process.

I'll not venture into any effort to make this part of the code base consistent, though. Thanks for your input.