Re: RFS: triggerhappy
Dies schrieb Niels Thykier (niels@thykier.net):
> Though the upstream code might need a little attention (or some
> clarification); e.g. cppcheck finds a resource leak:
>
> Checking cmdsocket.c...
> [cmdsocket.c:126]: (error) Resource leak: fd
>
> Looking at the code I am not sure what the intention is. My best guess
> is that it is trying to transmit the "leaked" file descriptor.
Yes, it does that. This allows the daemon itself to run as user "nobody",
while th-cmd (launched by udev) can open a new device and pass the fd
to thd.
> Though I am not convinced this will work; even if it is possible (I
> vaguely recall seeing an extension in Linux to allow something like
> this), I suspect the program relies on undefined behaviour here:
>
> char buffer[CMSG_SPACE(sizeof(dev_fd))];
> m.msg_control = buffer;
>
> The issue is that buffer goes out of scope before m is used (in sendmsg).
I guess you are refering to line 115 in cmdsocket.c?
I see, I'll move the buffer declaration to a safer location.
> In the if in uinput.c:27 it looks like you are missing a uinput_fd = -1;
Fixed that, I'm now simply calling close_uinput(..) there.
> In trigger.c:205 there is no check for if fork failed and this leads to
> a "silent nothing happened". There are also got a number of unchecked
> malloc/strdup, which could lead to "out of memory" segfaults, but at
> least that is "some kind of feedback".
OK, I added some error reporting there.
> cppcheck also believes that count_triggers (trigger.c) and print_ignores
> (ignore.c) are never used. I did not verify that.
Indeed, I used those functions for debugging purposes only, but I guess
leaving them there does no harm.
> In thd.c:331 the error message implies that the user does not exist, but
> getpwnam can fail with other reasons; it might be a good idea to
> use a more "neutral" error like:
> int err = errno;
> fprintf(stderr, "Could not look up the user %s\n", user);
> errno = err;
> perror("getpwnam");
OK, I "neutralized" the error message, however using perror(errno) from getpwnam
is not really interesting, since all I get is "Sucess", even when specifying a completely
bogus name.
> Adding warnings to gcc I saw the following:
> cc -g -O2 -Wall -Wextra -g -O2 -Wall -Wextra -c -o eventnames.o
> eventnames.c
> In file included from eventtable.h:10,
> from eventnames.c:5:
> evtable_KEY.h:123: warning: initialized field overwritten
> evtable_KEY.h:123: warning: (near initialization for 'KEY_NAME[122]')
[...]
> They are were triggered with -Wextra; it may be nothing, but I figured I
> would mention it.
Yes, I was aware of those. evtable_{KEY,EV,SW} are autogenerated from <linux/input.h>,
generating a mapping table from event names to the integer values, but since some events
have multiple names, these messages appear.
Thanks for your review, I'll push the changed advocated by you to my git repository
today (https://github.com/wertarbyte/triggerhappy).
Reply to: