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

Fix issue #1726: memory leaks in error code paths fix proposal. #531

Open
wants to merge 3 commits into
base: xdebug_2_9
from

Conversation

@devnexen
Copy link
Contributor

devnexen commented Dec 8, 2019

No description provided.

Copy link
Contributor

derickr left a comment

Thanks for looking into this!

The places to free memory have been correctly identified, but I've issue with the way how they have been addressed. I've added a few pointers.

In general, I would also like to see test cases for each of these situations, and preferably have the valgrind output that demonstrates these errors as part of the bug report. I am keen on making sure current bugs are covered by test cases, so that the same issue will not get reintroduced. I would also suggest to have at least one commit per specific memory leaking instance, in case having a distinct xdebug bugs issue is too much overkill (which I think it is, in this case).

cheers,
Derick

@@ -440,6 +440,7 @@ static xdebug_str* return_eval_source(char *id, int begin, int end)
xdebug_arg_dtor(parts);
return joined;
}
xdfree(parts);

This comment has been minimized.

Copy link
@derickr

derickr Dec 8, 2019

Contributor

Although this probably fixes the symptom, I would have fixed this in the following way:

  • Change xdebug_arg_dtor(parts) from a macro into new function (it's in usefulstuff.h/usefulstuff.c).
  • Create a new function xdebug_arg_ctor(), which does the xdmalloc on line 430, and the xdebug_arg_init(parts) in line 437.
  • Move the allocation on line 430 to within the xdebug_hash_find() { ... } block, so that parts only gets allocated when the element is found in the hash, and make it use the new xdebug_arg_ctor instead of xdmalloc + xdebug_arg_init.

The same issue as this one also exists in breakpoint_brk_info_fetch and breakpoint_remove.

@@ -875,6 +876,7 @@ DBGP_FUNC(breakpoint_set)
brk_info->hit_condition = XDEBUG_HIT_DISABLED;

if (!CMD_OPTION_SET('t')) {
xdfree(brk_info);

This comment has been minimized.

Copy link
@derickr

derickr Dec 8, 2019

Contributor

I would instead fix this in the following way:

  • Convert the setup sequence from line 857 to 876 into a new function xdebug_brk_info_ctor() and add it to handlers.c/handlers.h
  • Instead of the two additions of xdfree(brk_info), use xdebug_brk_info_dtor() instread.
} else {
xdfree(tmp_computerized_context);
return NULL;
}

This comment has been minimized.

Copy link
@derickr

derickr Dec 8, 2019

Contributor

Instead of fixing it this way, I would do the following:

  • Create a temp variable FILE *tmp_file;

  • Before the xdmalloc(sizeof(xdebug_trace_computerized_context));, call xdebug_trace_open_file(fname...); and assign the return value to tmp_file;

  • Then if the tmp_file is not NULL, return NULL.

  • Only then do lines 35-37, and instead assign tmp_computerized_context->trace_file to tmp_file.

  • I would also consider:

    • Changing the xdmalloc into a new xdebug_trace_computerized_context_ctor() function

    • Instead of just xdfree, create a new xdebug_trace_computerized_context_dtor() function that would:

      • free the allocated used_fname (xdebug_trace_open_file allocates that through xdebug_fopen and xdebug_open_file)
      • close the FILE handler, and set trace_file = NULL
      • free the structure itself.
      • Use this new function in xdebug_trace_computerized_deinit
} else {
xdfree(tmp_html_context);
return NULL;
}

This comment has been minimized.

Copy link
@derickr

derickr Dec 8, 2019

Contributor

See comment for computerized.

} else {
xdfree(tmp_textual_context);
return NULL;
}

This comment has been minimized.

Copy link
@derickr

derickr Dec 8, 2019

Contributor

See comment for computerized.

@devnexen devnexen force-pushed the devnexen:issue1726 branch from 0655948 to 8c2dbb1 Dec 8, 2019
@devnexen devnexen force-pushed the devnexen:issue1726 branch from 8c2dbb1 to 0743c57 Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.