[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv6 2/5] virstring.h/c: Util method for making some find and replace in strings



On Thu, Jan 23, 2014 at 10:28:30AM +0100, Manuel VIVES wrote:
> ---
>  src/libvirt_private.syms |    1 +
>  src/util/virstring.c     |   66 +++++++++++++++++++++++++++++++++++++++++++++-
>  src/util/virstring.h     |    1 +
>  3 files changed, 67 insertions(+), 1 deletion(-)

Can you also add a test case to src/virstringtest.c for this one
too. In particular test what happens when matches are exactly at
the start and end of the string  so we validate proper boundary
handling.

> +
> +/*
> + virStrReplace(haystack, oldneedle, newneedle) --
> +  Search haystack and replace all occurences of oldneedle with newneedle.
> +  Return a string with all the replacements in case of success, NULL in case
> +  of failure
> +*/
> +char *
> +virStrReplace(char *haystack,
> +              const char *oldneedle, const char *newneedle)

Can we call this  virStringReplace instead.

> +{
> +    char *ret = NULL;
> +    size_t i = strlen(haystack);
> +    size_t count = 0;
> +    size_t newneedle_len = strlen(newneedle);
> +    size_t oldneedle_len = strlen(oldneedle);
> +    int diff_len = newneedle_len - oldneedle_len;
> +    size_t totalLength = 0;
> +    size_t numberOfElementsToAllocate = 0;
> +    if (!haystack || !oldneedle || !newneedle) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("null value for haystack, oldneedle or newneedle"));
> +        goto cleanup;
> +    }
> +    if (oldneedle_len == 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("oldneedle cannot be empty"));
> +        goto cleanup;
> +    }
> +    if (diff_len == 0 && memcmp(oldneedle, newneedle, newneedle_len) == 0) {
> +        ignore_value(VIR_STRDUP(ret, haystack));
> +        goto cleanup;
> +    }
> +
> +    for (i = 0; haystack[i] != '\0'; i++) {
> +        if (memcmp(&haystack[i], oldneedle, oldneedle_len) == 0) {

This looks like a out of bounds read. eg consider

   haystack = "foo"
   oldneedle = "baaaaaar"

You're going to be making memcmp read beyond the end of the haystack
array I believe.

> +            count ++;
> +            i += oldneedle_len - 1;
> +        }
> +    }
> +    numberOfElementsToAllocate = (i + count * (newneedle_len - oldneedle_len));
> +
> +    if (VIR_ALLOC_N(ret, numberOfElementsToAllocate) < 0)
> +        goto cleanup;

Rather than trying to pre-caculate buffer lengths, I thin it would
probably be simpler if you just used the virBuffer APIs to construct
the new string, since that does grow-on-demand.
> +
> +    totalLength = sizeof(char) * numberOfElementsToAllocate;
> +    i = 0;
> +    while (*haystack) {
> +        if (memcmp(haystack, oldneedle, oldneedle_len) == 0) {
> +            if (virStrcpy(&ret[i], newneedle, totalLength) == NULL) {
> +                VIR_FREE(ret);
> +                goto cleanup;
> +            }
> +            totalLength -= newneedle_len;
> +            i += newneedle_len;
> +            haystack += oldneedle_len;
> +        } else {
> +            ret[i++] = *haystack++;
> +            totalLength --;
> +        }
> +    }
> +    ret[i] = '\0';
> +cleanup:
> +    return ret;
> +}


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]