Bug #12765
closedfwflash plugin cleanup handling is buggy
100%
Description
The fwflash
program has two different options for clean up handling. By default it will handle it itself, otherwise it will delegate it to a plugin that is of the appropriate version that declares the right interface. Unfortunately, there are several problems here:
- The mapfiles for v2 plugins don't actually include the required symbol to declare the plugin API version. This causes the symbol to be locally scoped and means that fwflash will never find it in a plugin (due its use of searching for it via dlsym). This in turn means that the clean functions of v2 plugins are never called.
- fwflash uses tailq's from sys/queue.h throughout. Unfortunately, it is removing items from the tail queue lists, but it is using the wrong macro to do so. While this isn't as clear in our manual pages as it should be, if you're going to remove an item from a tailq via
TAILQ_REMOVE
it is not correct to do so while iterating with theTAILQ_FOREACH
macro. You need to useTAILQ_FOREACH_SAFE
. This probably has worked by fluke because of the next problem.
- fwflash never freed the plugin or discovered devices. Presumably because doing so would have caused the tailq logic to crash, which I discovered when I finally got version 2 clean up working. If we have a version two plugin, it should be its responsibility to free the device and if not, then fwflash should do so. It never freed plugins, but there's no reason it can't.
This doesn't eliminate all of the memory leaks from fwflash, but at least has significantly trimmed down the list and made it so that a plugin is no longer responsible for any of the remaining leaks.
Updated by Robert Mustacchi almost 2 years ago
Updated by Robert Mustacchi almost 2 years ago
This was tested functionally by using the various fwflash -l and fwflash -r paths. In addition, I manually loaded libumem and checked for memory leaks at program termination. This ended up with a large reduction in leaks, though it did not completely eliminate them. However, the ones that remained were not introduced by these changes.
Updated by Electric Monk almost 2 years ago
- Status changed from New to Closed
- % Done changed from 90 to 100
git commit 8d55b80625b903a8ec6c560f6a38b5c16d1f5cfc
commit 8d55b80625b903a8ec6c560f6a38b5c16d1f5cfc Author: Robert Mustacchi <rm@fingolfin.org> Date: 2020-06-12T16:43:33.000Z 12759 Want ability to read ufm images 12758 ufm_detach doesn't properly clean up 12760 igb ufm support 12765 fwflash plugin cleanup handling is buggy Reviewed by: Patrick Mooney <patrick.mooney@joyent.com> Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com> Approved by: Joshua Clulow <josh@sysmgr.org>