Klocwork is reporting an error in URI_FUNC(MakeOwner) in UriNormalize.c. The relevant code is this:
URI_TYPE(PathSegment) * walker = uri->pathHead;
...
URI_TYPE(PathSegment) * ranger = uri->pathHead;
while (ranger->next != walker) {
...
if (...) {
...
free(ranger);
}
...
}
while (walker != NULL) {
URI_TYPE(PathSegment) * const next = walker->next;
...
}
The first time the call to free() is done, the thing being freed is uri->pathHead. So when walker->next is evaluated, walker is pointing at freed memory.
Hello Chris,
thanks for the report and explanation! The bug should be fixed by commit
https://sourceforge.net/p/uriparser/git/ci/07d5a0ccb462b754e245290c7404f127f2bab08a/tree/src/UriNormalize.c?diff=c035376bab8098aa2bfc6d173fe3d456b5050c73
and in upcoming release 0.8.3.
The change you have made doesn’t look right to me. The routine URI_FUNC(MakeOwner) now does this:
If execution enters the first if, the first while, and the second if, then ranger is set to uri->pathHead, which means that it’s the same as walker. Then you do while (ranger != walker). This will always be false. We never come back to the second while test, because of the return URI_FALSE. So (ranger != walker) will be tested once, and it will always be false. That doesn’t look right to me.
Hello Chris,
thanks for sharing your findings. I will need a few days to get a chance at a closer look.
Best, Sebastian
Hello again,
I had a closer look now. The "(ranger != walker)" loop is run if MakeRangeOwner failed (for out of memory). In that case, previous allocations (from MakeRangeOwner) are reverted: All those before current walker. So the "(ranger != walker)" loop processes all elements before current walker. The in-code comments are in line with that. If walker is at the start, no reverts are needed and that's expected. Looks good to me.
I scratched my head for a long time, and then at last I saw it. At the start of MakeOwner, walker is set equal to uri->pathHead. When execution first arrives at “while (walker != NULL)”, neither of them has changed. But at the end of the while loop is “walker = walker->next;”. So if after that we ever get to “ranger = uri->pathHead;” (because a call to MakeRangeOwner in the while loop has failed) then walker will no longer be equal to uri->pathHead, and so the “(ranger != walker)” test will not always be false.
Hello agian Chris,
to pull the best first: I agree: You found a memleak bug, awesome!
Depending on the code reader's view, the two clean-up loops could either (a) focus on a single task -- freeing text in a segment or freeing the segment itself -- or (b) limit themselves to processing segments that the other loop is not due to process so there is no overlap.
When I wrote the code my intention was (b), my guess is you read the code through the glassed of (a).
A fix in world (a) would be to:
A fix in world (b) would be to:
I went for the fix sticking to world (b), commit af233ea582b547ea56e274ef70c9d11a96da9f49.
Please let me know if you consider the bug fixed.
Thanks for your persistance!
Best
Sebastian
I don’t know how you got from my comments to spotting the memory leak, but it was you that saw it, not me. So Klocwork found the first problem (freed memory is looked at), that was fixed by your earlier change. And then you found the second problem (memory leak), and your latest change looks good to fix that. I saw my name in the change log, so thanks for that.