Fix some UB in signal handling #18
Conversation
sigaction(2) reads the sa_mask and sa_flags struct members as well, but we didn't initialize them so far. We probably didn't run into problems, because we allocate it when the stack is fresh and full of zeroes. Fix this by explicitly zeroing the struct and emptying the signal set. Signed-off-by: Ahmad Fatoum <[email protected]>
We use malloc elsewhere in the code with signals unmasked, but use free in a signal handler. This could result in a dead lock when terminating microcom by signal. The duplication of exit(0) call is ok, as the call site in the signal handler will be replaced in a follow-up commit. Signed-off-by: Ahmad Fatoum <[email protected]>
Both printf and exit aren't async signal safe, replace them with write and _Exit respectively. The access to ios and ios->exit and callees is still illegal, but at least we don't run risk reentering stdio and causing a deadlock anymore. As exit flushes stdio buffers as well, we now risk losing unflushed stdio output. This is acceptable as we aren't buffering the serial port output and the program is being terminated abnormally anyway. Signed-off-by: Ahmad Fatoum <[email protected]>
|
Sorry, github UI problems. I only intended to approve the first commit. |
There was a problem hiding this comment.
Hmm, I don't particularly like this. I'd like to allow backends calling async-unsafe functions. Freeing ios in global code seems to work, but is a layer violation because the structure was allocated in the backends. I'll think about that, if you want to improve this while I think that would be welcome of course :-)
ukleinek
dismissed
their stale review
added by accident, only intended to approve first commit
We could
Is it? |
While looking over #15, I noticed some issues with the way microcom handles signals. This fixes what was straight forward to fix. What remains is the access to
ios,ios->exitand whatever is called there. With #15 merged, this could be fixed by peppering somevolatilearound, but it's very unlikely to happen so postponing this for now.