Page MenuHomeFreeBSD

libiscsiutil: Fix a memory leak with negotiation keys.
ClosedPublic

Authored by jhb on Dec 17 2021, 8:58 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 1, 8:44 AM
Unknown Object (File)
Sun, Dec 1, 8:44 AM
Unknown Object (File)
Sun, Dec 1, 8:44 AM
Unknown Object (File)
Sun, Dec 1, 8:44 AM
Unknown Object (File)
Thu, Nov 28, 8:56 PM
Unknown Object (File)
Nov 11 2024, 9:52 PM
Unknown Object (File)
Nov 4 2024, 4:08 AM
Unknown Object (File)
Oct 22 2024, 3:17 PM
Subscribers

Details

Summary

When keys are loaded from a received PDU, a copy of the received keys
block is saved in the keys struct and the name and value pointers
point into that saved block. Freeing the keys frees this block.

However, when keys are added to a keys struct to build a set of keys
later sent in a PDU, the keys data block pointer is not used and
individual key names and values hold allocated strings. When the keys
structure was freed, all of these individual key name and value
strings were leaked.

Instead, allocate copies of strings for names and values when parsing
a set of keys from a received PDU and free all of the individual key
name and value strings when deleting a set of keys.

Sponsored by: Chelsio Communications

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.Dec 17 2021, 8:58 PM

Reading through the description my first though was that you are going to use different free routing for receive and transmit, depending on presence of keys_data, but since this is not time-critical code, this one is fine except the comment inline.

lib/libiscsi/keys.c
77 ↗(On Diff #100236)

I am not getting the point of malloc()+memcpy() to just traverse through it and then free().

lib/libiscsi/keys.c
77 ↗(On Diff #100236)

Because strsep modifies the buffer and I'm assuming the input buffer can't / shouldn't be modified. If this used something like strch instead with strndup then the copy could be avoided, but doing that is typically more fragile to write vs using strsep. Given this isn't a hot path, the extra malloc seems unimportant.

This revision is now accepted and ready to land.Dec 20 2021, 8:58 PM
jhb retitled this revision from libiscsi: Fix a memory leak with negotiation keys. to libiscsiutil: Fix a memory leak with negotiation keys..Dec 21 2021, 12:28 AM
  • Rebase after library rename.
This revision now requires review to proceed.Dec 21 2021, 12:41 AM
This revision was not accepted when it landed; it landed in state Needs Review.Dec 22 2021, 6:43 PM
This revision was automatically updated to reflect the committed changes.