immunant / IA2-Phase2

5 stars 0 forks source link

Rewriter generates incorrect `IA2_ADDR` invocations for null checks #411

Open randomPoison opened 1 month ago

randomPoison commented 1 month ago

If we have some pre-rewriter code that does fn_ptr == 0, this gets rewritten to IA2_ADDR(fn_ptr) == IA2_ADDR((fn_struct)0). The latter IA2_ADDR is incorrect, since the "address" being taken is a literal, rather than a function ptr struct. It would be better if the rewriter could detect this case and not emit the incorrect IA2_ADDR expression.

rinon commented 1 month ago

Can you add a test for this, please?

ayrtonm commented 1 month ago

IIUC this was a case we didn't consider in #198. Basically we turn function pointers into structs which can't be compared for equality so FnPtrEq::run turns both sides into addresses using IA2_ADDR and IA2_FN_ADDR. In the case where one side is null we implicitly assume that NULL is a macro so the pass does not modify that side. When code has fn_ptr == 0 it incorrectly tries to rewrite the rhs. I'll need to write a test case to check if other passes (e.g. FnPtrNull) also come into play here.

ayrtonm commented 1 month ago

I'm handling this together with #412 but I can't reproduce this part. In my new tests cases 3 and 4 do get rewritten as expected.

diff --git a/tests/rewrite_fn_ptr_eq/main.c b/tests/rewrite_fn_ptr_eq/main.c
index f8fe55e22..8a5c7f2b0 100644
--- a/tests/rewrite_fn_ptr_eq/main.c
+++ b/tests/rewrite_fn_ptr_eq/main.c
@@ -78,4 +78,19 @@ Test(rewrite_fn_ptr_eq, main) {
     if (y || !fn) { }
     // REWRITER: if (x && IA2_ADDR(fn) && y) { }
     if (x && fn && y) { }
+
+    // REWRITER: fn = (typeof(fn)) { NULL };
+    fn = NULL;
+
+    // REWRITER: fn = (typeof(fn)) { 0 };
+    fn = 0;
+
+    // REWRITER: if (IA2_ADDR(fn) == 0) { }
+    if (fn == 0) { }
+
+    // REWRITER: if (IA2_ADDR(mod.fn) == 0) { }
+    if (mod.fn == 0) { }
+
+    // REWRITER: res = 0;
+    res = 0;
 }
randomPoison commented 1 month ago

Maybe putting a cast before the 0 matters? An example case in zlib is

if (strm->zalloc == (alloc_func)0) {}

which gets rewritten as

if (IA2_ADDR(strm->zalloc) == IA2_ADDR((alloc_func)0)) {}
ayrtonm commented 1 month ago

yeah the explicit cast is the issue.