Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
axxel opened this issue Sep 10, 2024 · 2 comments
Open

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

axxel opened this issue Sep 10, 2024 · 2 comments

Comments

@axxel
Copy link
Contributor

axxel commented Sep 10, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

axxel commented Sep 12, 2024

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.

axxel added a commit to axxel/libgphoto2 that referenced this issue Oct 14, 2024
To be able to use the array macros in PTP and GP error "contexts", this
makes translate_ptp_result() tolerate both "types" of errors.

I added a comment regarding the use of the GP_ERROR_NO_MEMORY use. This is
related to gphoto#1008.
msmeissn pushed a commit that referenced this issue Oct 14, 2024
To be able to use the array macros in PTP and GP error "contexts", this
makes translate_ptp_result() tolerate both "types" of errors.

I added a comment regarding the use of the GP_ERROR_NO_MEMORY use. This is
related to #1008.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants