fix: libcontainer/handler.go potential memory leak

[cl0udee]

fix issue: #3446

Use the strings.Clone function to make devName no longer reference fields[0].

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[k8s-ci-robot]

Hi @cl0udee. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dims]

/ok-to-test

[dims]

/assign @bobbypage

[bobbypage]

Can you please explain why this fixes the memory leak? Why does cloning the string fix the issue? Can you provide a pprof maybe?

[cl0udee]

Can you please explain why this fixes the memory leak? Why does cloning the string fix the issue? Can you provide a pprof maybe?

Here are two pprof files that analyze the memory usage of the code:

The first one is the unoptimized original code.

ROUTINE ======================== github.com/google/cadvisor/container/libcontainer.scanInterfaceStats in vendor\github.com\google\cadvisor\container\libcontainer\handler.go
   56.35MB    16.67GB (flat, cum) 62.00% of Total
         .          .    478:   }
         .          .    479:   return false
         .          .    480:}
         .          .    481:
         .          .    482:func scanInterfaceStats(netStatsFile string) ([]info.InterfaceStats, error) {
         .   512.01kB    483:   file, err := os.Open(netStatsFile)
         .          .    484:   if err != nil {
         .          .    485:           return nil, fmt.Errorf("failure opening %s: %v", netStatsFile, err)
         .          .    486:   }
         .          .    487:   defer file.Close()
         .          .    488:
         .          .    489:   scanner := bufio.NewScanner(file)
         .          .    490:
         .          .    491:   // Discard header lines
         .          .    492:   for i := 0; i < 2; i++ {
         .          .    493:           if b := scanner.Scan(); !b {
         .          .    494:                   return nil, scanner.Err()
         .          .    495:           }
         .          .    496:   }
         .          .    497:
         .          .    498:   stats := []info.InterfaceStats{}
         .          .    499:   for scanner.Scan() {
         .       24MB    500:           line := scanner.Text()
         .    16.54GB    501:           line = strings.Replace(line, ":", "", -1)
         .          .    502:
         .    53.51MB    503:           fields := strings.Fields(line)
         .          .    504:           // If the format of the  line is invalid then don't trust any of the stats
         .          .    505:           // in this file.
         .          .    506:           if len(fields) != 17 {
         .          .    507:                   return nil, fmt.Errorf("invalid interface stats line: %v", line)
         .          .    508:           }
         .          .    509:
         .          .    510:           devName := fields[0]
         .          .    511:           if isIgnoredDevice(devName) {
         .          .    512:                   continue
         .          .    513:           }
         .          .    514:
         .          .    515:           i := info.InterfaceStats{
         .          .    516:                   Name: devName,
         .          .    517:           }
         .          .    518:
         .          .    519:           statFields := append(fields[1:5], fields[9:13]...)
         .          .    520:           statPointers := []*uint64{
         .          .    521:                   &i.RxBytes, &i.RxPackets, &i.RxErrors, &i.RxDropped,
         .          .    522:                   &i.TxBytes, &i.TxPackets, &i.TxErrors, &i.TxDropped,
         .          .    523:           }
         .          .    524:
         .          .    525:           err := setInterfaceStatValues(statFields, statPointers)
         .          .    526:           if err != nil {
         .          .    527:                   return nil, fmt.Errorf("cannot parse interface stats (%v): %v", err, line)
         .          .    528:           }
         .          .    529:
   56.35MB    56.35MB    530:           stats = append(stats, i)
         .          .    531:   }
         .          .    532:
         .          .    533:   return stats, nil
         .          .    534:}
         .          .    535:

From line 501, it can be observed that the variable line consumes a significant amount of memory, reaching up to 16.54GB. Such high memory usage should not be expected. The reason behind this is that on line 510, devName references line[0], preventing it from being garbage collected.

To prevent the aforementioned case, use strings.Clone to generate a new copy. Then, devName references the new copy, allowing the original line to be reclaimed. The pprof code is as follows:

ROUTINE ======================== github.com/google/cadvisor/container/libcontainer.scanInterfaceStats in vendor\github.com\google\cadvisor\container\libcontainer\handler.go
  905.88MB   923.43MB (flat, cum) 22.93% of Total
         .          .    482:func scanInterfaceStats(netStatsFile string) ([]info.InterfaceStats, error) {
         .          .    483:   file, err := os.Open(netStatsFile)
         .          .    484:   if err != nil {
         .          .    485:           return nil, fmt.Errorf("failure opening %s: %v", netStatsFile, err)
         .          .    486:   }
         .          .    487:   defer file.Close()
         .          .    488:
         .          .    489:   scanner := bufio.NewScanner(file)
         .          .    490:
         .          .    491:   // Discard header lines
         .          .    492:   for i := 0; i < 2; i++ {
         .    14.05MB    493:           if b := scanner.Scan(); !b {
         .          .    494:                   return nil, scanner.Err()
         .          .    495:           }
         .          .    496:   }
         .          .    497:
         .          .    498:   stats := []info.InterfaceStats{}
         .          .    499:   for scanner.Scan() {
         .   512.07kB    500:           line := scanner.Text()
         .   512.10kB    501:           fmt.Println(line)
         .   512.07kB    502:           line = strings.Replace(line, ":", "", -1)
         .          .    503:           fmt.Println(line)
         .          .    504:           fields := strings.Fields(line)
         .        2MB    505:           fmt.Println(fields)
         .          .    506:           // If the format of the  line is invalid then don't trust any of the stats
         .          .    507:           // in this file.
         .          .    508:           if len(fields) != 17 {
         .          .    509:                   return nil, fmt.Errorf("invalid interface stats line: %v", line)
         .          .    510:           }
         .          .    511:
         .          .    512:           //devName := fields[0]
  557.51MB   557.51MB    513:           devName := strings.Clone(fields[0])
         .          .    514:           if isIgnoredDevice(devName) {
         .          .    515:                   continue
         .          .    516:           }
         .          .    517:
         .          .    518:           i := info.InterfaceStats{
         .          .    519:                   Name: devName,
         .          .    520:           }
         .          .    521:
         .          .    522:           statFields := append(fields[1:5], fields[9:13]...)
         .          .    523:           statPointers := []*uint64{
         .          .    524:                   &i.RxBytes, &i.RxPackets, &i.RxErrors, &i.RxDropped,
         .          .    525:                   &i.TxBytes, &i.TxPackets, &i.TxErrors, &i.TxDropped,
         .          .    526:           }
         .          .    527:
         .          .    528:           err := setInterfaceStatValues(statFields, statPointers)
         .          .    529:           if err != nil {
         .          .    530:                   return nil, fmt.Errorf("cannot parse interface stats (%v): %v", err, line)
         .          .    531:           }
         .          .    532:
         .          .    533:           stats = append(stats, i)
  348.37MB   348.37MB    534:   }
         .          .    535:
         .          .    536:   return stats, nil
         .          .    537:}
         .          .    538:
         .          .    539:func setInterfaceStatValues(fields []string, pointers []*uint64) error {

[cl0udee]

@bobbypage Hi, I hope everything is going well. Just wanted to check in and see if there's any update on the progress of this Pull Request. Thank you!