#28 Klocwork reporting an error

[Chris Hills]

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.

[Sebastian Pipping]

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.

[Chris Hills]

The change you have made doesn’t look right to me. The routine URI_FUNC(MakeOwner) now does this:

static URI_INLINE UriBool URI_FUNC(MakeOwner)(URI_TYPE(Uri) * uri,
        unsigned int * doneMask) {
    URI_TYPE(PathSegment) * walker = uri->pathHead;
    ...
    code that doesn’t change walker, or uri, or uri->pathHead
    ...
    if ((*doneMask & URI_NORMALIZE_PATH) == 0) {
        while (walker != NULL) {
            if (!URI_FUNC(MakeRangeOwner)(doneMask, 0, &(walker->text))) {
                URI_TYPE(PathSegment) * ranger = uri->pathHead;
                while (ranger != walker) {
                    ...
                }
                ...
                return URI_FALSE; /* Raises malloc error */
            }
            walker = walker->next;
        }
        *doneMask |= URI_NORMALIZE_PATH;
    }
    ...

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.

[Sebastian Pipping]

Hello Chris,

thanks for sharing your findings. I will need a few days to get a chance at a closer look.

Best, Sebastian

[Sebastian Pipping]

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.

[Chris Hills]

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.

[Sebastian Pipping]

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:

• reset walker to uri->pathHead right before the second loop so that all segments are freed by the second loop
• remove "free(ranger);" from the first loop to not free twice

A fix in world (b) would be to:

• Move "free(ranger);" out of the ranger->text guard

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

[Chris Hills]

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.