Bug#700738: valgrind summary
On 2013-03-08 22:25:46, Antoine Beaupré wrote: I really wonder what to do at this point. I can certainly upload the 2.0 version to experimental to allow people to test this more thoroughly (but then again, it's just once C file, easy enough to test). But I don't feel those bugs are serious enough to block the Wheezy release. It doesn't seem those issues are critical enough to justify the serious severity, but maybe I am wrong. Would I would like to do is to upload a 1.1-2 with Bremner's patch and then request an unblock and close this bug report. Even with David's patch and ttyclock and sig initialized (see the attached patch), there are some issues left: the incorrect type of ttyclock-running and calling ncurses stuff in the signal handler. Anyway, signals intermixed with ncurses is very much out of my comfort zone. Maybe Thorsten (CCed) can provide additional input on those issues. Regards -- Sebastian Ramacher diff --git a/ttyclock.c b/ttyclock.c index 6df69e6..15e8151 100644 --- a/ttyclock.c +++ b/ttyclock.c @@ -58,6 +58,7 @@ init(void) refresh(); /* Init signal handler */ + sigemptyset(sig.sa_mask); sig.sa_handler = signal_handler; sig.sa_flags = 0; sigaction(SIGWINCH, sig, NULL); @@ -445,6 +446,7 @@ main(int argc, char **argv) /* Alloc ttyclock */ ttyclock = malloc(sizeof(ttyclock_t)); + memset(ttyclock, 0, sizeof(ttyclock_t)); /* Date format */ ttyclock-option.format = malloc(sizeof(char) * 100); @@ -478,14 +480,14 @@ main(int argc, char **argv) break; case 'i': puts(TTY-Clock 2 © by Martin Duquesnoy (xor...@gmail.com)); - free(ttyclock); free(ttyclock-option.format); + free(ttyclock); exit(EXIT_SUCCESS); break; case 'v': puts(TTY-Clock 2 © devel version); - free(ttyclock); free(ttyclock-option.format); + free(ttyclock); exit(EXIT_SUCCESS); break; case 's': @@ -527,8 +529,8 @@ main(int argc, char **argv) key_event(); } - free(ttyclock); free(ttyclock-option.format); + free(ttyclock); endwin(); return 0; signature.asc Description: Digital signature
Bug#700738: valgrind summary
Sebastian Ramacher dixit: Anyway, signals intermixed with ncurses is very much out of my comfort zone. Maybe Thorsten (CCed) can provide additional input on those issues. Sorry, no, no practical experience either way, but it did raise all alarm bells here while reading it. bye, //mirabilos -- „nein: BerliOS und Sourceforge sind Plattformen für Projekte, github ist eine Plattform für Einzelkämpfer“ -- dieses Zitat ist ein Beweis dafür, daß auch ein blindes Huhn mal ein Korn findet, bzw. – in diesem Fall – Recht haben kann -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#700738: valgrind summary
On 2013-03-06 20:02:16, Antoine Beaupré wrote: So I ran the patched version under valgrind, which I am not familiar with at all so YMMV. I attach the output. Basically, what I see is one of those: ==3852== Conditional jump or move depends on uninitialised value(s) ==3852== Use of uninitialised value of size 8 ==3852== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) This seems consistent with the original report of problems with the signal handler, in general, but I haven't dug much deeper yet. Those valgrind warnings can be fixed quite easily. valgrind is complaining because some members of ttyclock malloc'd in main and sigaction in init are read before they were initialized. For example, in line 70 it is checked if ttyclock-geo.x is zero, but ttyclock-geo.x has no well defined value at this point. To silence the warnings it should be enough to just run memset over the allocated memory: once for ttyclock after the first malloc in main and once for sig in init. Regards -- Sebastian Ramacher signature.asc Description: Digital signature
Bug#700738: valgrind summary
On 2013-03-08, Sebastian Ramacher wrote: On 2013-03-06 20:02:16, Antoine Beaupré wrote: So I ran the patched version under valgrind, which I am not familiar with at all so YMMV. I attach the output. Basically, what I see is one of those: ==3852== Conditional jump or move depends on uninitialised value(s) ==3852== Use of uninitialised value of size 8 ==3852== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) This seems consistent with the original report of problems with the signal handler, in general, but I haven't dug much deeper yet. Those valgrind warnings can be fixed quite easily. valgrind is complaining because some members of ttyclock malloc'd in main and sigaction in init are read before they were initialized. For example, in line 70 it is checked if ttyclock-geo.x is zero, but ttyclock-geo.x has no well defined value at this point. To silence the warnings it should be enough to just run memset over the allocated memory: once for ttyclock after the first malloc in main and once for sig in init. So it turns out that there's a new upstream at github that worked a bit on the code, at the very least to cleanup the use-after-free problems. But there are other changes on that branch, including providing a manpage and fixing the readme, so that may not be fit for a release... Here's the diffstat: Makefile| 18 +- README | 24 ++-- tty-clock.1 | 121 + ttyclock.c | 93 ++--- ttyclock.h | 10 ++ 5 files changed, 240 insertions(+), 26 deletions(-) But almost all warnings are fixed in that 2.0 release out there, only those remain: ==14964== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) ==14964==at 0x52AC60E: __libc_sigaction (sigaction.c:65) ==14964==by 0x401A6B: init (ttyclock.c:75) ==14964==by 0x4033D5: main (ttyclock.c:593) ==14964== Address 0x7fefffc18 is on thread 1's stack ==14964== ==14964== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) ==14964==at 0x52AC60E: __libc_sigaction (sigaction.c:65) ==14964==by 0x401A84: init (ttyclock.c:76) ==14964==by 0x4033D5: main (ttyclock.c:593) ==14964== Address 0x7fefffc18 is on thread 1's stack ==14964== ==14964== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) ==14964==at 0x52AC60E: __libc_sigaction (sigaction.c:65) ==14964==by 0x401A9D: init (ttyclock.c:77) ==14964==by 0x4033D5: main (ttyclock.c:593) ==14964== Address 0x7fefffc18 is on thread 1's stack ==14964== ==14964== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) ==14964==at 0x52AC60E: __libc_sigaction (sigaction.c:65) ==14964==by 0x401AB6: init (ttyclock.c:78) ==14964==by 0x4033D5: main (ttyclock.c:593) ==14964== Address 0x7fefffc18 is on thread 1's stack I really wonder what to do at this point. I can certainly upload the 2.0 version to experimental to allow people to test this more thoroughly (but then again, it's just once C file, easy enough to test). But I don't feel those bugs are serious enough to block the Wheezy release. It doesn't seem those issues are critical enough to justify the serious severity, but maybe I am wrong. Would I would like to do is to upload a 1.1-2 with Bremner's patch and then request an unblock and close this bug report. Another option would be to upload upload 2.0-1 to sid and try to unblock *that*, but I feel this would take too much time from the release managers. Any opinions on the way to go forward here? Are the signal handlers warnings important enough to block the wheezy release? Also note that I have forwarded to our new upstream, we'll see what happens there.. Thanks for the feedback, A. -- Information is not knowledge Knowledge is not wisdom Wisdom is not truth - Frank Zappa pgp9yE8w7LqOE.pgp Description: PGP signature
Bug#700738: valgrind summary
So I ran the patched version under valgrind, which I am not familiar with at all so YMMV. I attach the output. Basically, what I see is one of those: ==3852== Conditional jump or move depends on uninitialised value(s) ==3852== Use of uninitialised value of size 8 ==3852== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) This seems consistent with the original report of problems with the signal handler, in general, but I haven't dug much deeper yet. What I am concerned about is making this fit for release. As this is not a server application or a suid app, I am not sure I see how serious those problems really are. The software actually works even though it's coding is clunky and some edge cases will make it crash. But it is not a security liability, so I think this should be downgraded to normal and therefore unblock the release. Or am I missing something? A. ==3852== Memcheck, a memory error detector ==3852== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al. ==3852== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info ==3852== Command: ./tty-clock ==3852== Parent PID: 31646 ==3852== ==3852== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) ==3852==at 0x52AC60E: __libc_sigaction (sigaction.c:65) ==3852==by 0x401567: init (ttyclock.c:63) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== Address 0x7fefffdb8 is on thread 1's stack ==3852== ==3852== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) ==3852==at 0x52AC60E: __libc_sigaction (sigaction.c:65) ==3852==by 0x401580: init (ttyclock.c:64) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== Address 0x7fefffdb8 is on thread 1's stack ==3852== ==3852== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) ==3852==at 0x52AC60E: __libc_sigaction (sigaction.c:65) ==3852==by 0x401599: init (ttyclock.c:65) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== Address 0x7fefffdb8 is on thread 1's stack ==3852== ==3852== Syscall param rt_sigaction(act-sa_mask) points to uninitialised byte(s) ==3852==at 0x52AC60E: __libc_sigaction (sigaction.c:65) ==3852==by 0x4015B2: init (ttyclock.c:66) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== Address 0x7fefffdb8 is on thread 1's stack ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x4015CC: init (ttyclock.c:70) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x4015E8: init (ttyclock.c:72) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x401604: init (ttyclock.c:74) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x401620: init (ttyclock.c:76) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x401643: init (ttyclock.c:78) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x53182DB: __tzfile_compute (tzfile.c:642) ==3852==by 0x5318036: __tz_convert (tzset.c:627) ==3852==by 0x40167D: init (ttyclock.c:80) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x53182EC: __tzfile_compute (tzfile.c:672) ==3852==by 0x5318036: __tz_convert (tzset.c:627) ==3852==by 0x40167D: init (ttyclock.c:80) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x5318319: __tzfile_compute (tzfile.c:715) ==3852==by 0x5318036: __tz_convert (tzset.c:627) ==3852==by 0x40167D: init (ttyclock.c:80) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Use of uninitialised value of size 8 ==3852==at 0x5318516: __tzfile_compute (tzfile.c:718) ==3852==by 0x5318036: __tz_convert (tzset.c:627) ==3852==by 0x40167D: init (ttyclock.c:80) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x531851A: __tzfile_compute (tzfile.c:718) ==3852==by 0x5318036: __tz_convert (tzset.c:627) ==3852==by 0x40167D: init (ttyclock.c:80) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Conditional jump or move depends on uninitialised value(s) ==3852==at 0x5318524: __tzfile_compute (tzfile.c:720) ==3852==by 0x5318036: __tz_convert (tzset.c:627) ==3852==by 0x40167D: init (ttyclock.c:80) ==3852==by 0x402C2C: main (ttyclock.c:520) ==3852== ==3852== Use of uninitialised value of size 8 ==3852==at 0x531852A: __tzfile_compute (tzfile.c:720) ==3852==by
