commaai / msgq

MSGQ: A lock free single producer multi consumer message queue
175 stars 252 forks source link

Properly checking the return value of ipc_sendrecv_with_fds #624

Open deanlee opened 2 months ago

deanlee commented 2 months ago

This PR addresses an issue where the VIPC server closes the socket if the required stream type is unavailable: https://github.com/commaai/msgq/blob/23cb05a80dec50a52f2e0fa7f7870ae920b3f2e8/msgq/visionipc/visionipc_server.cc#L136-L140 This causes the ipc_sendrecv_with_fds function in the VIPC client to return 0. However, the client does not properly handle this error and instead relies on two assertions that are always true: https://github.com/commaai/msgq/blob/23cb05a80dec50a52f2e0fa7f7870ae920b3f2e8/msgq/visionipc/visionipc_client.cc#L56-L59

This PR introduces a proper check to ensure that the return value of ipc_sendrecv_with_fds is correctly handled if it is less than or equal to 0. These changes resolve the following bugs:

1. Assertion in getAvailableStreams

the vipc client still can connect to the ipc path even if the camerad is not running. This could be due to the camerad not shutting down properly and the ipc path not being removed.

ipc_sendrecv_with_fds will returns -1 In this case. this causes the following assertion to fail.

ui: cereal/visionipc/visionipc_client.cc:133: static std::set VisionIpcClient::getAvailableStreams(const std::string &, bool): Assertion `(r >= 0) && (r % sizeof(VisionStreamType) == 0)' failed.

This bug can be reproduced by clicking “preview driver camera" button quickly and continuously in the UI interface.

2. vipcClient:connect will returns true even if an attempt is made to connect to a non-existent stream type the ipc_sendrecv_with_fds will return 0 if required stream type is not available. return false in this case.

This bug can be reproduced by run watch3, and run replay --demo . quit replay after the road camera displayed in watch3. and run replay --demo --ecam --dcam. the watch3 will assert and quit:

watch3: msgq_repo/msgq/visionipc/visionipc_client.cc:96: VisionBuf VisionIpcClient::recv(VisionIpcBufExtra , const int): Assertion `packet->idx < num_buffers' failed.

3. crash in VideoWidget::vipcAvailableStreamsUpdated:

2 0x00007f882bce5729 in __assert_fail_base (fmt=0x7f882be7b588 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5618e8 "", file=0x5617ff "repeat-1", line=263, function=) at assert.c:92

3 0x00007f882bcf6fd6 in __GI___assert_fail (assertion=0x5618e8 "", file=0x5617ff "repeat-1", line=263, function=0x5618be "\311?") at assert.c:101

4 0x000000000047ebce in QString::QString(char const*) (this=0x7fffde080230, ch=0x2 <error: Cannot access memory at address 0x2>) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:700

5 VideoWidget::vipcAvailableStreamsUpdated(std::set<VisionStreamType, std::less, std::allocator >) (this=Python Exception <class 'gdb.MemoryError'> Cannot access memory at address 0x0:

0x6546a8 <StreamNotifier::instance()::notifier+8>, streams=#6 0x000000000047e9a2 in VideoWidget::loopPlaybackClicked() (this=) at tools/cabana/videowidget.cc:198

7 0x0000000001bb68e0 in ()

8 0x0000000001bb68b0 in ()

9 0x0000000001eb9680 in ()

10 0x0000000001c4ef10 in ()

11 0x00007fffde080240 in ()

12 0x00007f882cca5b40 in () at /lib/x86_64-linux-gnu/libQt5Core.so.5

13 0x00007f882ca10328 in QMetaObject::activate(QObject*, int, int, void**) () at /lib/x86_64-linux-gnu/libQt5Core.so.5