Undefined pointer comparison in macro.c

[DemiMarie]

GCC has -fsanitize=pointer-compare, which adds instrumentation to detect invalid pointer comparisons. When built with this flag, and run with ASAN_OPTIONS=detect_invalid_pointer_pairs=1, virtually the entire testsuite fails due to an undefined pointer comparison in rpmio/macro.c. This is in turn due to q being initialized as one before the beginning of an allocated region.

Reproduction script below. PR coming.

build-rpm.gz

[pmatilai]

That is probably undefined behavior in C11 but AIUI not in C99, which rpm uses.
Perhaps I wasn't clear enough in my earlier comment today: as a rule of thumb, I'm not interested in undefined behavior findings, exactly because of it's totally undefined what standard it's checking. Unless it reveals a concrete bug of course.

[DemiMarie]

That is probably undefined behavior in C11 but AIUI not in C99, which rpm uses.

It is undefined in C99; see http://www.open-std.org/jtc1/sc22/wg14/www/C99RationaleV5.10.pdf, §6.3.2.3:

Regardless how an invalid pointer is created, any use of it yields undefined behavior.

So I still believe it should be fixed.

Perhaps I wasn't clear enough in my earlier comment today: as a rule of thumb, I'm not interested in undefined behavior findings, exactly because of it's totally undefined what standard it's checking. Unless it reveals a concrete bug of course.

This is undefined behavior in C99, and includes a reproducible test case. I consider that to be a concrete bug.

[mlschroe]

Maybe I'm dense, but isn't q incremented in line 221 before the pointer comparison is done?

[DemiMarie]

It gets decremented again on line 227.

[mlschroe]

So the initialization is not the problem? And a fix would be do change the for loop to:

for (; nb && p <= q; p++) {

[DemiMarie]

So the initialization is not the problem? And a fix would be do change the for loop to:

for (; nb && p <= q; p++) {

The initialization is already undefined behavior.

[mlschroe]

The if (nb == 0 in line 253 is most likely wrong as it is true for empty lines. I guess it should read:

if ((nb && *q != '\\' && !bc && !pc && !xc) || *(q+1) == '\0') {

Panu, do you agree?

[mlschroe]

I also don't think rpm will work correctly on EBCDIC platforms.

[mlschroe]

Of course the initialization can be easily fixed by doing a

*q = 0;  /* terminate */

at the top of the do loop and

q++;    /* move forward */

at the bottom. That way, q can be initialized to buf. (It will be decremented to buf-1 for empty lines nevertheless.)

[mlschroe]

Regarding my comment about the wrong nb check:

The following macro definition works:

%hello %{expand:
hi
ho
}

But this produces an error:

%hello %{expand:
hi

ho
}

[mlschroe]

My suggested fix was wrong, it should be:

if (((!nb || *q != '\\') && !bc && !pc && !xc) || *(q+1) == '\0') {

Back to the pointer comparison: an even saner approach for this issue would be to let q point to the newline and not the char before the newline.

[pmatilai]

This is actually a wonderful example: just like compiler warnings, undefined behavior is best viewed as a canary that points to buggy code, rather than flaw in itself. Much, much damage to codebases around the world is done by "fixing" compiler warnings, and undefined behavior is not any different.

@mlschroe 's version fixes an ages old, very concrete bug (even an RFE) in the macro parser. It wasn't misbehaving because of the allegedly undefined behavior but because it was just a piece of bad old code, led to by the undefined behavior alert.

Fixed by #1606