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

catch all the possible reallocs that could return a NULLto later be… #193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

psi29a
Copy link
Member

@psi29a psi29a commented May 14, 2018

…dereferrenced

Try catching issues raised here:
#192 (comment)

@psi29a
Copy link
Member Author

psi29a commented May 14, 2018

@sezero any thoughts or comments?

@sezero
Copy link
Contributor

sezero commented May 14, 2018

  • WM_ERR_CORUPT seems wrong. Should possibly be WM_ERR_MEM (or whatever is there for it.)
  • exit() from the lib is BAD: some error return from the relevant procedure is needed.
  • upon realloc() failure, old ptr is not free()d and leaked. we need maybe something like:
  type *tmp = (type *) realloc(buf, newsize);
  if (!tmp) {
   /* handle error */
  }
  else {
    buf = tmp;
  }

@psi29a
Copy link
Member Author

psi29a commented May 14, 2018

I'm heading off to bed, feel free to commit into this pull request if you see something that should be fixed. I'll catch up with this tomorrow. Thanks for reviewing.

update: Back...

@sezero
Copy link
Contributor

sezero commented May 15, 2018

cb85b69: Made it to compile c89-only compilers.

I still don't like this. We are not really handling errors. Not really.
See, e.g. _WM_CheckEventMemoryPool(): it just returns, no one
checks failure,and for this particular case the old pointer still is
not freed.

This needs more more work, and I believe it isn't that urgent.

@psi29a
Copy link
Member Author

psi29a commented May 15, 2018

No, it isn't urgent. If a library can't reallocate memory, then we're screwed anyway. A null dereference will cause a seg fault (or something similar in windows), which is worse than having a library just 'exit'. So I can understand 'logging' the issue, but we still need to warn the application about the issue so that it can be aware of the issue.

@sezero sezero added this to the 0.4.5 milestone Mar 21, 2021
@sezero sezero modified the milestones: 0.4.5, 0.4.6 Jan 8, 2023
@psi29a
Copy link
Member Author

psi29a commented Apr 1, 2024

This PR is ancient.. not sure if it's still worth it or will need re-work later? Otherwise I close

@sezero
Copy link
Contributor

sezero commented Apr 1, 2024

This PR is ancient.. not sure if it's still worth it or will need re-work later? Otherwise I close

As you wish.

@sezero
Copy link
Contributor

sezero commented Apr 3, 2024

I rebased this myself (and even did a minor correction.) It compiles but
suffers from returned value check issues:

  • internal_midi.c: _WM_CheckEventMemoryPool returns void, therefore
    the failure isn't known to its callers. (some of its callers do return
    a value themselves, but do their callers check in return??)

  • patches.c: _WM_load_patch returns void therefore the failure isn't
    known to its callers.

  • mus2mid.c,xmi2mid.c: resize_dst returns void therefore the failure
    isn't known to its callers.

If you are interested in this patch, those issues need to be resolved.

@sezero sezero modified the milestones: 0.4.6, 0.4.7 Apr 11, 2024
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

Successfully merging this pull request may close these issues.

2 participants