[libvirt] [PATCH] test: fix segfault in networkxml2argvtest

[Osier Yang]

On 2012年04月05日 15:32, Laine Stump wrote:
> This bug resolves https://bugzilla.redhat.com/show_bug.cgi?id=810100
>
> rpm builds for i686 were failing with a segfault in
> networkxml2argvtest. Running under valgrind showed that a region of
> memory was being referenced after it had been freed (as the result of
> realloc - see the valgrind report in the BZ).
>
> The problem (in replaceTokens() - added in commit 22ec60, meaning this
> bug was in 0.9.10 and 0.9.11) was that the pointers token_start and
> token_end were being computed based on the value of *buf, then *buf
> was being realloc'ed (potentially moving it), then token_start and
> token_end were used without recomputing them to account for movement
> of *buf.
>
> The solution is to change the code so that token_start and token_end
> are offsets into *buf rather than pointers. This way there is only a
> single pointer to the buffer, and nothing needs readjusting after a
> realloc. (You may note that some uses of token_start/token_end didn't
> need to be changed to add in "*buf +" - that's because there ended up
> being a +*buf and -*buf which canceled each other out).
>
> DV gets the credit for finding this bug and pointing out the valgrind
> report.
> ---
>   tests/networkxml2argvtest.c |   13 +++++++------
>   1 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c
> index cf00181..87519e4 100644
> --- a/tests/networkxml2argvtest.c
> +++ b/tests/networkxml2argvtest.c
> @@ -18,18 +18,19 @@
>   /* Replace all occurrences of @token in @buf by @replacement and adjust size of
>    * @buf accordingly. Returns 0 on success and -1 on out-of-memory errors. */
>   static int replaceTokens(char **buf, const char *token, const char *replacement) {
> -    char *token_start, *token_end;
> +    size_t token_start, token_end;
>       size_t buf_len, rest_len;
>       const size_t token_len = strlen(token);
>       const size_t replacement_len = strlen(replacement);
>       const int diff = replacement_len - token_len;
>
>       buf_len = rest_len = strlen(*buf) + 1;
> -    token_end = *buf;
> +    token_end = 0;
>       for (;;) {
> -        token_start = strstr(token_end, token);
> -        if (token_start == NULL)
> +        char *match = strstr(*buf + token_end, token);
> +        if (match == NULL)
>               break;
> +        token_start = match - *buf;
>           rest_len -= token_start + token_len - token_end;
>           token_end = token_start + token_len;
>           buf_len += diff;
> @@ -37,8 +38,8 @@ static int replaceTokens(char **buf, const char *token, const char *replacement)
>               if (VIR_REALLOC_N(*buf, buf_len)<  0)
>                   return -1;
>           if (diff != 0)
> -            memmove(token_end + diff, token_end, rest_len);
> -        memcpy(token_start, replacement, replacement_len);
> +            memmove(*buf + token_end + diff, *buf + token_end, rest_len);
> +        memcpy(*buf + token_start, replacement, replacement_len);
>           token_end += diff;
>       }
>       /* if diff<  0, we could shrink the buffer here... */


Looks good, ACK.