| Summary: | printf format width mismatch segfault in ARM disassembly | ||
|---|---|---|---|
| Product: | binutils | Reporter: | Stephen McCamant <smcc> |
| Component: | binutils | Assignee: | unassigned |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | dave, nickc, will.newton |
| Priority: | P2 | ||
| Version: | 2.22 | ||
| Target Milestone: | --- | ||
| Host: | Target: | ||
| Build: | Last reconfirmed: | ||
| Attachments: |
Patch to fix printf argument types
more printf format fixes |
||
Here's a short command sequence to reproduce (in the appropriate configuration):
% perl -e 'print pack("V*", 0xed69cdcb)' >/tmp/bad.insn
% objdump -b binary -m arm -EL -D /tmp/bad.insn
The expected result is something like:
0: ed69cdcb stcl 13, cr12, [r9, #-812]! ; 0xfffffcd4
whereas what I see instead is a segmentation fault.
Hi Stephen, I think I've also hit this case. Watch out though with reenabling that attribute check that there are a few broken printf's elsewhere; mips-dis.c seems to have a load of int's passed to long's. (I'll attach a patch tomorrow or so). (A friend managed to type random hex and hit cd45b43a which segs in this way on ARM). Dave Created attachment 6543 [details]
more printf format fixes
This diff includes Stephen's diff and also patches mips (that has mismatches)
and hppa, 68k and sparc that used printf without a format string, and newer gcc's seem to be (rightly) picky about that.
make check seems OK with this, but it should get checked by guys who know those arch.
CVSROOT: /cvs/src Module name: src Changes by: nickc@sourceware.org 2012-07-24 12:56:48 Modified files: opcodes : ChangeLog arm-dis.c hppa-dis.c m68k-dis.c microblaze-dis.c mips-dis.c ppc-dis.c sparc-dis.c include : ChangeLog dis-asm.h Log message: PR binutils/13135 * arm-dis.c: Add necessary casts for printing integer values. Use %s when printing string values. * hppa-dis.c: Likewise. * m68k-dis.c: Likewise. * microblaze-dis.c: Likewise. * mips-dis.c: Likewise. * ppc-dis.c: Likewise. * sparc-dis.c: Likewise. * dis-asm.h (fprintf_ftype): Add ATTRIBUTE_FPTR_PRINTF_2. Patches: http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/ChangeLog.diff?cvsroot=src&r1=1.1828&r2=1.1829 http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/arm-dis.c.diff?cvsroot=src&r1=1.151&r2=1.152 http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/hppa-dis.c.diff?cvsroot=src&r1=1.50&r2=1.51 http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/m68k-dis.c.diff?cvsroot=src&r1=1.37&r2=1.38 http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/microblaze-dis.c.diff?cvsroot=src&r1=1.4&r2=1.5 http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/mips-dis.c.diff?cvsroot=src&r1=1.91&r2=1.92 http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/ppc-dis.c.diff?cvsroot=src&r1=1.58&r2=1.59 http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/sparc-dis.c.diff?cvsroot=src&r1=1.23&r2=1.24 http://sourceware.org/cgi-bin/cvsweb.cgi/src/include/ChangeLog.diff?cvsroot=src&r1=1.580&r2=1.581 http://sourceware.org/cgi-bin/cvsweb.cgi/src/include/dis-asm.h.diff?cvsroot=src&r1=1.86&r2=1.87 Patch approved and applied. Cheers Nick CVSROOT: /cvs/src Module name: src Changes by: nickc@sourceware.org 2012-07-26 14:05:38 Modified files: include : ChangeLog opcodes : ChangeLog Log message: Fix attributation of PR 13135 patch. Patches: http://sourceware.org/cgi-bin/cvsweb.cgi/src/include/ChangeLog.diff?cvsroot=src&r1=1.581&r2=1.582 http://sourceware.org/cgi-bin/cvsweb.cgi/src/opcodes/ChangeLog.diff?cvsroot=src&r1=1.1830&r2=1.1831 The original issue appears to be fixed, so marking as resolved. |
Created attachment 5913 [details] Patch to fix printf argument types The change to using bfd_vma for offset computations, described in the fix for PR/12752 (CVS revision 1.146 of opcodes/arm-dis.c) causes the code to crash in configurations where the size of bfd_vma is different than the size of "int", because the code is now passing values of type bfd_vma to a "%d" format specifier. For instance one such code snippet in print_insn_coprocessor looks like: 1876 bfd_vma offset = given & 0xff; ... 1892 if (offset) 1893 func (stream, ", #%d]%s", 1894 offset, 1895 WRITEBACK_BIT_SET ? "!" : ""); When this code passes "offset" as a 64-bit value, the printf function will interpret the low 32 bits as the %d argument, and the high 32 bits as the %s argument, but if the value is negative, the high bits will be equivalent to -1, which causes a segfault when used as character pointer. For instance, I see this when I compile a version of the binutils that supports 32-bit x86, 64-bit AMD64, and ARM on a 32-bit x86/Linux host system; then "int" is 32 bits, but bfd_vma is 64 bits. This is the sort of error that is supposed to be caught by GCC's format string checking. I see that that checking was enabled at the relevant place in 2005 (change 1.54 to include/dis-asm.h), but then it was disabled again (perhaps inadvertently; I don't see anything about it in the log message) in 2007 (change 1.67 to the same file). I've attached a proof-of-concept patch which re-enables the warnings, and then adds casts on all the printf arguments in arm-dis.c that cause warnings under this configuration. I've verified that this fixes all the crashes I've seen in my configuration; objdump can now disassembly 50MB of random bytes without crashing. However I haven't investigated whether this problem occurs elsewhere, and I haven't checked whether this respects all the signs correctly, which was the issue in PR/12752. (For instance, could there be a configuration where bfd_vma is 32 bits but int is 64?)