Reproducer: ``` %macro x 1+k %endmacro x a, ``` This file triggers an invalid read and write after the paramlen array. ``` $ valgrind ./nasm -felf64 -g -FDWARF -o /tmp/asd repro ==3083202== Memcheck, a memory error detector ==3083202== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al. ==3083202== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info ==3083202== Command: ./nasm -felf64 -g -FDWARF -o /tmp/asd repro ==3083202== ==3083202== Invalid read of size 4 ==3083202== at 0x426E71: expand_mmacro (preproc.c:6633) ==3083202== by 0x41B902: pp_tokline (preproc.c:7316) ==3083202== by 0x41AF04: pp_getline (preproc.c:7328) ==3083202== by 0x403CD4: assemble_file (nasm.c:1722) ==3083202== by 0x402F9C: main (nasm.c:717) ==3083202== Address 0x4af91b8 is 0 bytes after a block of size 8 alloc'd ==3083202== at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==3083202== by 0x407ADC: nasm_calloc (alloc.c:72) ==3083202== by 0x426C93: expand_mmacro (preproc.c:6588) ==3083202== by 0x41B902: pp_tokline (preproc.c:7316) ==3083202== by 0x41AF04: pp_getline (preproc.c:7328) ==3083202== by 0x403CD4: assemble_file (nasm.c:1722) ==3083202== by 0x402F9C: main (nasm.c:717) ==3083202== ==3083202== Invalid write of size 4 ==3083202== at 0x426E74: expand_mmacro (preproc.c:6633) ==3083202== by 0x41B902: pp_tokline (preproc.c:7316) ==3083202== by 0x41AF04: pp_getline (preproc.c:7328) ==3083202== by 0x403CD4: assemble_file (nasm.c:1722) ==3083202== by 0x402F9C: main (nasm.c:717) ==3083202== Address 0x4af91b8 is 0 bytes after a block of size 8 alloc'd ==3083202== at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==3083202== by 0x407ADC: nasm_calloc (alloc.c:72) ==3083202== by 0x426C93: expand_mmacro (preproc.c:6588) ==3083202== by 0x41B902: pp_tokline (preproc.c:7316) ==3083202== by 0x41AF04: pp_getline (preproc.c:7328) ==3083202== by 0x403CD4: assemble_file (nasm.c:1722) ==3083202== by 0x402F9C: main (nasm.c:717) ==3083202== repro:4: warning: dropping trailing empty parameter in call to multi-line macro `x' [-w+macro-params-legacy] ==3083202== ==3083202== HEAP SUMMARY: ==3083202== in use at exit: 4,480 bytes in 12 blocks ==3083202== total heap usage: 3,806 allocs, 3,794 frees, 851,774 bytes allocated ==3083202== ==3083202== LEAK SUMMARY: ==3083202== definitely lost: 4,141 bytes in 6 blocks ==3083202== indirectly lost: 0 bytes in 0 blocks ==3083202== possibly lost: 0 bytes in 0 blocks ==3083202== still reachable: 339 bytes in 6 blocks ==3083202== suppressed: 0 bytes in 0 blocks ==3083202== Rerun with --leak-check=full to see details of leaked memory ==3083202== ==3083202== For lists of detected and suppressed errors, rerun with: -s ==3083202== ERROR SUMMARY: 8 errors from 2 contexts (suppressed: 0 from 0) ```
I think this bug might be a duplicate of another heap overflow: https://bugzilla.nasm.us/show_bug.cgi?id=3392743 But this one comes with a minimal test case.
After reading the code, to me it looks like the bug lies on the logic to process empty macro arguments, in is_mmacro (preproc.c) ``` free_tlist(*comma); *comma = NULL; if (raw_nparam > found->nparam_min && raw_nparam <= found->nparam_min + found->ndefs) { /* Replace empty argument with default parameter */ params[raw_nparam] = found->defaults[raw_nparam - found->nparam_min]; ``` This is assigning the last param as the default, but that is inconsistent with what the documentation says the default params are for. This is assigning params[raw_nparam] which is different than *nparamp (the value returned to the caller). If the behavior that we want is that the macro takes a, k as parameters, then we should do something like: ``` *nparamp = nparam = raw_nparam; ``` Right after setting params[raw_nparam]. However, further down the code it seems like a warning is triggered: ``` } else if (comma) { nasm_warn(WARN_MACRO_PARAMS_LEGACY, "dropping trailing empty parameter in call to multi-line macro `%s'", found->name); } ``` Which would imply that the expected behavior would be to NOT add the default value as the last parameter.