Bug 1205288

Let's backport wholesale:

7f303ad clean unnecessary code: don't check g_strdup arg for NULL
2458810 Drop superfluous conditionals around g_strdup()
44c2286 util: Drop superfluous conditionals around g_free()
9be71be util: Fuse g_malloc(); memset() into g_new0()
c89c6e8 util/uri: uri_new() can't fail, drop dead error handling
afd5ea3 util/uri: realloc2n() can't fail, drop dead error handling
afb30dd util/uri: URI member path can be null, compare more carfully
2b21233 util/uri: Add overflow check to rfc3986_parse_port

Fix included in qemu-kvm-0.12.1.2-2.474.el6

I ran local Coverity scans for qemu-kvm-0.12.1.2-2.473.el6 (unpatched)
qemu-kvm-0.12.1.2-2.475.el6 and (patched).

csdiff reports two out of three defects in uri.c fixed:

Error: REVERSE_INULL:
uri.c:1988: deref_ptr_in_call: Dereferencing pointer "bas->path".
uri.c:1992: check_after_deref: Null-checking "bas->path" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

Error: REVERSE_INULL:
uri.c:1988: deref_ptr_in_call: Dereferencing pointer "ref->path".
uri.c:1996: check_after_deref: Null-checking "ref->path" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.

Unchanged:

Error: OVERRUN:
uri.c:2035: overrun-local: Overrunning array of 2 bytes at byte offset 2 by dereferencing pointer "ref->path + pos".

This is actually a false positive.  Coverity worries that after

    int pos = 0;

and

    if (ref->path == NULL) {
        ref->path = (char *) "/";
	remove_path = 1;
    }

we increment pos to 2 in

	if ((ref->path[pos] == '.') && (ref->path[pos+1] == '/'))
            pos += 2;

and then access ref->path[2] out of bounds in

	while ((bptr[pos] == ref->path[pos]) && (bptr[pos] != 0))
	    pos++;

Can't happen, because the conditional guarding pos += 2 can't be true:
ref->path[0] is '/', hence != '.'.

The defects we actually fixed are real.  Adjusting bug description.

I conclude that this is not a defect, but using a different analysis. Please review and let me know if you disagree.

pos starts at 0. The next reference to pos occurs in this statement:

    if ((ref->path[pos] == '.') && (ref->path[pos+1] == '/'))
        pos += 2;

For pos to be incremented to 2, the conditional must be true. So what can we say about ref->path? It is either:

a) the string "/" as assigned here:

    if (ref->path == NULL) {
        ref->path = (char *) "/";
        remove_path = 1;
    }

in which case the value of pos is not incremented. Or it is:

b) a string of unknown length (initialized from actual parameter 'uri'). So let's reason about the string and whether or not pos gets incremented by:

    if ((ref->path[pos] == '.') && (ref->path[pos+1] == '/'))
        pos += 2;

* It could be a string of length 0; that is,

    ref->path[0] == '\0'

If so, the expression 'ref->path[pos+1]' references undefined (and possibly unallocated) memory beyond the end of the string (a separate defect not noted by Coverity). pos won't be incremented because 'ref->path[pos]' is '\0' not '.'.

* It could be a string of length 1, but if so that means ref->path[1]=='\0'.  pos won't be incremented because '\0' != '/'.

* It could be a string of length 2, in which case ref->path[2]=='\0'. pos *might* be incremented because we don't know anything about ref->path[0] or ref->path[1].

* It could be a string of length > 2. pos *might* be incremented.

Now we come to the statement that Coverity claims is a defect:

    while ((bptr[pos] == ref->path[pos]) && (bptr[pos] != 0))
        pos++;

This statement is OK; the value 'ref->path[pos]' is valid for pos==2 because for pos to be 2, we know that the string has to be at least 2 characters long.

Finally, I happened to notice that uri_resolve_relative, the function in question, has NO callers. Code that doesn't execute can't have side effects, whether good or bad.

The remarks in Comment 9 are directed at the analysis in Comment 7, not at the BZ in general.

> I conclude that this is not a defect, but using a different
> analysis. Please review and let me know if you disagree.
>
> pos starts at 0. The next reference to pos occurs in this statement:
>
>     if ((ref->path[pos] == '.') && (ref->path[pos+1] == '/'))
>         pos += 2;
>
> For pos to be incremented to 2, the conditional must be true. So what
> can we say about ref->path? It is either:
>
> a) the string "/" as assigned here:
>
>     if (ref->path == NULL) {
>         ref->path = (char *) "/";
>         remove_path = 1;
>     }
>
> in which case the value of pos is not incremented. Or it is:
>
> b) a string of unknown length (initialized from actual parameter
> 'uri'). So let's reason about the string and whether or not pos gets
> incremented by:
>
>     if ((ref->path[pos] == '.') && (ref->path[pos+1] == '/'))
>         pos += 2;
>
> * It could be a string of length 0; that is,
>
>     ref->path[0] == '\0'
>
> If so, the expression 'ref->path[pos+1]' references undefined (and
> possibly unallocated) memory beyond the end of the string (a separate
> defect not noted by Coverity).

Since the right operand of && is executed only when the left operand
is true.  It isn't in this case, so there's no defect.

> pos won't be incremented because 'ref->path[pos]' is '\0' not '.'.

Correct.

> * It could be a string of length 1, but if so that means
>   ref->path[1]=='\0'.  pos won't be incremented because '\0' != '/'.

Correct.

> * It could be a string of length 2, in which case
>   ref->path[2]=='\0'. pos *might* be incremented because we don't know
>   anything about ref->path[0] or ref->path[1].

Correct.

> * It could be a string of length > 2. pos *might* be incremented.

Correct.

> Now we come to the statement that Coverity claims is a defect:
>
>     while ((bptr[pos] == ref->path[pos]) && (bptr[pos] != 0))
>         pos++;
>
> This statement is OK; the value 'ref->path[pos]' is valid for pos==2
> because for pos to be 2, we know that the string has to be at least 2
> characters long.

Correct.

> Finally, I happened to notice that uri_resolve_relative, the function
> in question, has NO callers. Code that doesn't execute can't have side
> effects, whether good or bad.

Good point :)

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-1275.html