bhyve: memory leak in topology_parse()
ClosedPublic
Actions

Authored by andy_omniosce.org on Feb 16 2022, 3:20 PM.

Details

Reviewers

rew
jhb

Group Reviewers

bhyve

Commits

rG62bed9d49e41: bhyve: plug memory leak in topology_parse()
rGad3da82996ff: bhyve: plug memory leak in topology_parse()

Summary

This was identified in the illumos port using the umem allocator:

umem_alloc_16 leak: 1 buffer, 16 bytes
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
          81e460           817fd0     570a4304d986                1
                           7d6668           79aa80                0
                 libc.so.1`strdup+0x25
                 topology_parse+0x2b
                 main+0x1c5

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 44524
Build 41412: arc lint + arc unit

Event Timeline

andy_omniosce.org created this revision.Feb 16 2022, 3:20 PM

Herald added a reviewer: bhyve. · View Herald TranscriptFeb 16 2022, 3:20 PM

Herald added subscribers: Contributor Reviews (src), bcran, rgrimes, imp. · View Herald Transcript

andy_omniosce.org requested review of this revision.Feb 16 2022, 3:20 PM

Harbormaster completed remote builds in B44479: Diff 102841.Feb 16 2022, 3:20 PM

corvink added a subscriber: corvink.Feb 16 2022, 3:27 PM

corvink added inline comments.

usr.sbin/bhyve/bootrom.c
205 ↗	(On Diff #102841)	Just as a note, this memory leak should be solved by my D33433 patch too.

Dropping the fix for bootrom.c

This is addressed by D33433

Harbormaster completed remote builds in B44524: Diff 103026.Feb 21 2022, 11:40 AM

andy_omniosce.org retitled this revision from bhyve: fix two small memory leaks to bhyve: memory leak in topology_parse().Feb 21 2022, 11:41 AM

andy_omniosce.org edited the summary of this revision. (Show Details)

what purpose is ostr serving here?

In D34301#777509, @rew wrote:

what purpose is ostr serving here?

It's keeping track of the address of the buffer that needs to be freed at the end. The problem with str is that it's used with strsep() which will change the pointer, leaving it eventually as NULL, and so the original version just did free(NULL); at best, and worse in the case that an unknown key was found.

rew accepted this revision as: rew.Feb 21 2022, 7:28 PM

This revision is now accepted and ready to land.Feb 21 2022, 7:28 PM

In some other places that use strsep the pattern is to name this variable tofree which might be a more obvious name than o[riginal]str.

Renamed 'ostr' to 'tofree'

This revision now requires review to proceed.Feb 24 2022, 1:25 PM

Harbormaster completed remote builds in B44564: Diff 103194.Feb 24 2022, 1:25 PM

In D34301#778155, @jhb wrote:

In some other places that use strsep the pattern is to name this variable tofree which might be a more obvious name than o[riginal]str.

Yes, that's better, thanks; updated.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 24 2022, 5:43 PM

Closed by commit rGad3da82996ff: bhyve: plug memory leak in topology_parse() (authored by andy_omniosce.org, committed by rew). · Explain Why

This revision was automatically updated to reflect the committed changes.

rew added a commit: rGad3da82996ff: bhyve: plug memory leak in topology_parse().

jhb added a commit: rG62bed9d49e41: bhyve: plug memory leak in topology_parse().Jan 26 2023, 7:26 PM