Project

General

Profile

Bug #12765

fwflash plugin cleanup handling is buggy

Added by Robert Mustacchi 7 months ago. Updated 6 months ago.

Status:
Closed
Priority:
Normal
Category:
cmd - userland programs
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

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 the TAILQ_FOREACH macro. You need to use TAILQ_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.

#2

Updated by Robert Mustacchi 6 months 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.

#3

Updated by Joshua M. Clulow 6 months ago

  • Gerrit CR set to 681
#4

Updated by Electric Monk 6 months 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>

Also available in: Atom PDF