neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15265 )
Change subject: fix: vty crash by logging to killed telnet session ...................................................................... Patch Set 1: (4 comments) https://gerrit.osmocom.org/#/c/15265/1//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/#/c/15265/1//COMMIT_MSG@11 PS1, Line 11: segfaults: the vty->obuf is already NULL. > Why do we try to send a message to a killed session first of all? That seems > to be the route of the […] I want to make sure that we cannot ever crash by writing to a NULL ofd. Whatever the reasons for the NULL ofd, and whatever the reasons for wanting to log to it. Fixing the higher level fail would be: close down the logging target before taking down the ofd. I care about that only secondarily. This is only one cause for logging to NULL that we found now. Fair enough, if we ensure closed telnets always close down log targets safely the issue would not exist, but I am not willing to verify that now. That is why I want these NULL checks at the buffer API level. It makes all other calling code bugs non-fatal in a trivial way, without the need to fully audit the ancient vty code. Can we please get over this and not make a huge thing of it? https://gerrit.osmocom.org/#/c/15265/1//COMMIT_MSG@16 PS1, Line 16: Also guard all buffer_*() functions against a NULL buffer argument, which > this looks like a hack to fix other parts of the code which may be wrong, and > looks like it should b […] If the calling code paths are not trivially clear, it is good practise to guard against NULL pointers. Rather drop a log line than crashing the entire program. https://gerrit.osmocom.org/#/c/15265/1//COMMIT_MSG@20 PS1, Line 20: Related: OS#4164 > This ticket doesn't look related to me. this one is correct, the one in the comment has a typo https://gerrit.osmocom.org/#/c/15265/1/src/vty/vty.c File src/vty/vty.c: https://gerrit.osmocom.org/#/c/15265/1/src/vty/vty.c@266 PS1, Line 266: * of this same (killed) telnet session. See OS#4146. */ > OS#4146 (https://osmocom. […] aha, the commit log has the correct one: #4164. Here there is a number swap. thx -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15265 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Idca3f54dc986abf6784790c12e69e02bdf77cb41 Gerrit-Change-Number: 15265 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-CC: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Tue, 27 Aug 2019 15:40:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
