t51 and t52 report "buffer overflow detected" when built with -D_FORTIFY_SOURCE enabled

[James McCoy]

I ran into this while working on the packaging for Debian.

From a fresh checkout:

$ env CFLAGS="-g -O2 -D_FORTIFY_SOURCE=2" make test
...
t/51tickit-timer.t ........ *** buffer overflow detected ***: /home/jamessan/src/leonerd.org.uk/libtickit/t/.libs/lt-51tickit-timer.t terminated
t/51tickit-timer.t ........ No subtests run
t/52tickit-later.t ........ *** buffer overflow detected ***: /home/jamessan/src/leonerd.org.uk/libtickit/t/.libs/lt-52tickit-later.t terminated
t/52tickit-later.t ........ No subtests run

$ libtool e gdb --args ./t/51tickit-timer.t
...
(gdb) run
Starting program: /home/jamessan/src/leonerd.org.uk/libtickit/t/.libs/lt-51tickit-timer.t
*** buffer overflow detected ***: /home/jamessan/src/leonerd.org.uk/libtickit/t/.libs/lt-51tickit-timer.t terminated

Program received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1 0x00007ffff783ccf7 in __GI_abort () at abort.c:90
#2 0x00007ffff787df87 in __libc_message (action=<optimized out>,
    fmt=fmt@entry=0x7ffff79839ee "*** %s ***: %s terminated\n")
    at ../sysdeps/posix/libc_fatal.c:181
#3 0x00007ffff790d7de in __GI___fortify_fail_abort (need_backtrace=need_backtrace@entry=true,
    msg=msg@entry=0x7ffff798396b "buffer overflow detected") at fortify_fail.c:33
#4 0x00007ffff790d811 in __GI___fortify_fail (
    msg=msg@entry=0x7ffff798396b "buffer overflow detected") at fortify_fail.c:44
#5 0x00007ffff790b6d0 in __GI___chk_fail () at chk_fail.c:28
#6 0x00007ffff790d71a in __fdelt_chk (d=d@entry=-1) at fdelt_chk.c:25
#7 0x00007ffff7bcad60 in tickit_term_input_wait_msec (tt=0x55555579aef0, msec=9)
    at src/term.c:683
#8 0x00007ffff7bce2b9 in tickit_run (t=0x55555579b000) at src/tickit.c:195
#9 0x0000555555555044 in main (argc=1, argv=0x7fffffffe7e8) at t/51tickit-timer.c:47
(gdb) frame 7
#7 0x00007ffff7bcad60 in tickit_term_input_wait_msec (tt=0x55555579aef0, msec=9)
    at src/term.c:683
683 FD_SET(fd, &readfds);
(gdb) l
678 timeout.tv_usec = (msec % 1000) * 1000;
679 }
680
681 fd_set readfds;
682 int fd = termkey_get_fd(tk);
683 FD_SET(fd, &readfds);
684 int ret = select(fd + 1, &readfds, NULL, NULL, msec > -1 ? &timeout : NULL);
685
686 if(ret == 0)
687 timedout(tt);
(gdb) p fd
$1 = -1

[James McCoy (jamessan)]

As suggested in the gdb logs, termkey_get_fd() is returning -1, which results in invalid indexing of readfds.

Steve Langasek supplied the attached patch to be more defensive about the value returned from termkey_get_key(). This avoids the invalid indexing when -1 is returned, but this part of the code should be reviewed to see if it's expected to get a -1 value.

[James McCoy (jamessan)]

Additionally, here's another patch from Steve which ensures the tests are built with the supplied CFLAGS/LDFLAGS.

[Paul "LeoNerd" Evans (leonerd)]

Thanks. Now applied (manually, with some splitting)