Skip to content
This repository has been archived by the owner on Jan 17, 2022. It is now read-only.

Fix resource leak in DPL_APPEND_STR macro #198

Open
Joacchim opened this issue Jun 4, 2015 · 3 comments
Open

Fix resource leak in DPL_APPEND_STR macro #198

Joacchim opened this issue Jun 4, 2015 · 3 comments

Comments

@Joacchim
Copy link
Contributor

Joacchim commented Jun 4, 2015

The commit 8b2288c by @QuentinPerez attempted to address the leak, but failed to properly fix the thing.

It actually fixed the leak by calling a function instead of a macro that returned an error in case of failure, and checking the return value of this macro in part of the places. The issue was that this macro was used in a number of places, using length types ranging from int, unsigned int, size_t and so on.

This led to another potential error in some non-fixed places in the droplet library: the implicit erroneous cast to size_t* from int* or unsigned int* meant that a memory overrun could easily occur on 64bits systems (which is the main target of this library)

The patch was a good idea, but incomplete, and must be reworked and improved.

@QuentinPerez
Copy link
Contributor

Hi @Joacchim,

Normally, I fixed it in my second PR #197 / 038528b ?

@Joacchim
Copy link
Contributor Author

Joacchim commented Jun 4, 2015

I think the second PR was never merged, so It was not part of my considerations for the revert. For the revert my consideration, more than the leak was to avoid any risk of silent failure, which is not completely/properly addressed by the reverted patch, and neither it is by the patch you mention.

ie, looking at the patch you point to quickly, I still see an issue: The DPL_APPEND_STR macro's returned value is not checked in those two files, creating a silent error.

What we want to make sure for this patch is:

  • Create a consistent use of the type of "length" variables throughout the codebase, in order for the DPL_APPEND_STR macro to become a function instead of a macro (we could as well remove the macro altogether for clarity).
  • Avoid any silent error: Check properly the returned status of the dpl_append_str function, in order to manage properly any failure that could arise (hence the leak fix)

Edit: Please understand that we have some ongoing process for testing on the side, and we cannot afford to have WIP patches in the master; we still think that fixing this potential leak is important (hence the issue)

Edit2: Also, please refrain from doing cosmetics in fix or feature patches (the whitespace removals). It is a kind of noise that makes reviewing patches much harder than it should be. Those cosmetic fixes should be part of cosmetics-only patches or of patches that actually modify the related part of the code.

@QuentinPerez
Copy link
Contributor

Nice, No problem I understand :)
And my bad for the whitespaces, it was my editor that removes it when I save a file.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants