Radare2: RVector bugs

[radare]

there are some more bugs in rpvector. but ill try to report them as separate issues

• vector grows linearly, when it should grow depending on the index if its larger than the next vector capacity (see below)

$ git diff
diff --git a/libr/util/vector.c b/libr/util/vector.c
index 9d406e9a4..095948f37 100644
--- a/libr/util/vector.c
+++ b/libr/util/vector.c
@@ -143,10 +143,13 @@ R_API void r_vector_remove_at(RVector *vec, size_t index, void *into) {

 R_API void *r_vector_insert(RVector *vec, size_t index, void *x) {
        r_return_val_if_fail (vec, NULL);
-       if (vec->len >= vec->capacity) {
+       if (R_MAX (vec->len, index) >= vec->capacity) {
                RESIZE_OR_RETURN_NULL (NEXT_VECTOR_CAPACITY);
        }
        void *p = r_vector_index_ptr (vec, index);
+       if (!p) {
+               return NULL;
+       }
        if (index < vec->len) {
                memmove ((char *)p + vec->elem_size, p, vec->elem_size * (vec->len - index));
        }

[thestr4ng3r]

The proposed fix is wrong but yes, this is a bug indeed.

[thestr4ng3r]

Or well it depends on how "insert" is interpreted. Right now it is correct if the index is only allowed to be in the range [0, vec->len]. I'm not sure if we really should allow more. Normally you should just use r_vector_reserve(), then r_vector_assign_at(). Insert will really "insert" and move the elements behind the index further.

[thestr4ng3r]

I wouldn't consider this as a bug as-is. As I have written it depends on the desired semantics of "insert".

[XVilka]

@trufae @thestr4ng3r any decision on this problem?

[ret2libc]

I don't think this is a bug indeed. As @thestr4ng3r already said you should use reserve and then insert or directly pushif you want to put elements in a vector. IMHO this can be closed.

[trufae]

i would say it's confusing and error-prone because the api assumes correct usage and imho it shuld check for nulls and bounds to avoid such mistakes like we do with all the other data structure apis

[thestr4ng3r]

Checking for bounds with r_return sounds good to me.

[trufae]

@thestr4ng3r would you do this change?

[thestr4ng3r]

I'll put it on my list. Might take a few days though until I can do it.