Page MenuHomeFreeBSD

bhyve: memory leak in topology_parse()
ClosedPublic

Authored by andy_omniosce.org on Feb 16 2022, 3:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 14, 4:17 PM
Unknown Object (File)
Tue, Dec 3, 3:03 AM
Unknown Object (File)
Oct 24 2024, 2:52 AM
Unknown Object (File)
Sep 30 2024, 11:31 PM
Unknown Object (File)
Sep 27 2024, 8:03 PM
Unknown Object (File)
Sep 24 2024, 8:49 AM
Unknown Object (File)
Sep 23 2024, 8:11 PM
Unknown Object (File)
Sep 23 2024, 9:21 AM

Details

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 44564
Build 41452: arc lint + arc unit

Event Timeline

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

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.

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

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.

This revision now requires review to proceed.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
This revision was automatically updated to reflect the committed changes.