Memguard Panics After Creating 15 Objects

[theory]

Program:

package main

import (
    "fmt"
    "github.com/libeclipse/memguard"
)

func main() {
    for i := 0; i < 100; i++ {
        key := []byte("123456768901234567890123456789012")
        safeKey, err := memguard.NewFromBytes(key, true)
        if err != nil {
            panic(err.Error())
        }
        fmt.Printf("%v: %v\n", i, safeKey)
    }
}

Output on a CentOS Linux release 7.3.1611 VM:

0: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
1: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
2: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
3: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
4: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
5: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
6: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
7: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
8: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
9: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
10: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
11: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
12: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
13: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
14: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
15: &{{0 0} [49 50 51 52 53 54 55 54 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50 51 52 53 54 55 56 57 48 49 50] true false}
panic: memguard.memcall.Lock(): could not acquire lock on 0x7f7711a67000 [Err: cannot allocate memory]

goroutine 1 [running]:
github.com/libeclipse/memguard/memcall.Lock(0x7f7711a67000, 0x1000, 0x2000)
    ~/go/src/github.com/libeclipse/memguard/memcall/memcall_unix.go:18 +0x16c
github.com/libeclipse/memguard.New(0x21, 0x4cd800, 0x21, 0x30, 0x21)
    ~/go/src/github.com/libeclipse/memguard/memguard.go:78 +0xf6
github.com/libeclipse/memguard.NewFromBytes(0xc420010bd0, 0x21, 0x30, 0xc420010b01, 0x21, 0x30, 0x0)
    ~/go/src/github.com/libeclipse/memguard/memguard.go:113 +0x34
main.main()
    try.go:11 +0x8a

It happens on every run on this host. I get the same results on a CentOS release 6.9 (Final) VM. It does not fail on my Mac.

[theory]

Note that it does not fail if I destroy each key. Seems odd to have a cap on the number of memguard objects one is allowed to have, though. :-(

[awnumar]

Looks like the call to mlock(2) is failing. This is expected behaviour, most OS only allow each process to mlock a certain amount of memory, unless of course the process is root. You can use ulimit -l to see the limit on your system.

It would probably be good to include a note about this limitation, or even look into automatically increasing it.

[theory]

$ ulimit -l
64

[theory]

Oh my Mac:

> ulimit -l
unlimited

Ha!

[josephlr]

@libeclipse changing the limit is usually a privileged operation, so automatic increasing might not work (and processes generally shouldn't change their limits). I think the better option is checking for ENOMEM (or equivalent) and replacing it with a better error message (as the default message is wrong here) that explains that the limit has been hit. Also, adding documentation about ulimt -l might be a good idea, as well as mentioning that the limit does not apply to privileged processes (at least on Linux)

@theory 4 byte keys * 16 keys = 64 bytes! So the 17th should fail! This actually might be a good test case.

[josephlr]

@libeclipse you could also add a finalizer to unlock the memory. This won't solve all the problems, but it will help too much memory from being locked if users forget to destroy their keys (as you are then having the garbage collector unlock the memory).

[theory]

@josephlr Glat do help. For my use, this issue came up because I have tests that generate a shitload of objects with keys, iterating over various use cases. Pasting in a slew of calls to DestroyAll() to fix my tests. At this point, I plan to have only two of these things in released code, but still, good to know, thanks!

[josephlr]

@theory For my code (which does something similar to memguard), I got in the habit of putting defer statements to wipe/destroy the keys right after creating them. This limits their lifetime and is less taxing on system resources.

[awnumar]

@josephlr I thought it'd need root, oh well. The process should still panic when the limit is hit, but improving the error message is trivial.

I don't understand what you mean by a finaliser though, that's what Destroy does. Do you mean do the GC's job? Remember that the GC can't touch our memory since it was allocated using system calls.

Oh and about the test case, (@theory) remember that we allocate and lock a multiple of the page size, and ulimit -l is rounded down to the nearest multiple of the page size.

[theory]

FWIW, I've been using a finalizer just to try to catch things I might otherwise miss:

import "runtime"
// ...
runtime.SetFinalizer(guardedKey, finalize)
 // ...
func finalize(key *memguard.LockedBuffer) {
    key.Destroy()
}

[theory]

But the finalizer only fires when GC kicks in. Which doesn't happen while my tests run, if I had to guess.

[josephlr]

For the LockedBuffer, the Buffer member is not managed by the Go GC, but the memory for the Locked Buffer is. Having the finalizer lets the Buffer be destroyed when the LockedBuffer is destroyed by the Go GC. The main problem is that the GC doesn't know or care about the locked memory limit, so it does not realize the system is "under pressure" in that regard. Having the finalizers is still better

And for the test case I messed up, 16 key * 4kb pages = 64kB (ulimit is always in kilobytes).

[awnumar]

@josephlr Yep. Another thing to consider is that there's no guarantee about when the GC will run. This can be dealt with a little by calling runtime.GC() (if it's worth it, it might not be and it's messy).

[josephlr]

@libeclipse I don't think running the GC here is the right move (as you said, it's messy). The best analogy here is file descriptors. Only a maximum number can be open at a time (set by ulimit -n and /proc/sys/fs/file-max). Go's "Solution" to this is just to have os.Open() fail with too many open files when it happens and encourage people to use file.Close() (either explicitly or in a defer statement). Mirroring that approach here seems the least confusing.

[josephlr]

For similar reasons, Go sets a finalizer to close its file descriptors. https://github.com/golang/go/blob/master/src/os/file_unix.go#L118

[awnumar]

@theory Just a note about your example in the OP, I'd use a []byte literal and skip allocating key. Reduces the chances of the GC getting its dirty mutts in there before we can wipe it.

[awnumar]

@josephlr Thought about it and I don't think that finalisers would work.

For a LockedBuffer to run out of scope, it would need to have zero references left to it. This can never happen for non-destroyed LockedBuffers, since we hold an array of pointers to all of them so that the DestroyAll function knows what it's doing. So unless there's some way of keeping track of all of the LockedBuffers without keeping references to them (short of implementing our own GC), I don't think adding them is possible.

[josephlr]

@libeclipse that's a good point. The only alternative would be having DestroyAll keep track of just the buffers and not the LockedBuffer structs, but that might not be worth the complexity.

I think encouraging defer statements would be enough.

[theory]

Can you hold an array of weak pointers, instead? Maybe just memory addresses? You'd have to be careful to have Destroy remove them, so that none of the remaining addresses would be unallocated when DestroyAll is called.

[awnumar]

@theory I thought about that but if the GC was to go ahead and move the LockedBuffer around in memory, our "soft" pointers wouldn't be updated. The GC can't mess with the actual buffer though, so I suppose we could hold pointers to them. Then again that would mess up some other functions like LockedBuffers().

I mean it's possible with some convoluted scheme, but is it worth it? I'm not convinced.

[awnumar]

I wonder, if we initiate a finaliser on the buffer itself, would that be able to run out of scope? Or would the reference to the LockedBuffer holding it prevent that?

[dotcppfile]

I'm not sure why this is a big deal, memguard is a library that's meant to be used by advanced programmers to securely allocate memory for a small amount of "keys", that's why there's a limit for that. Abusing that limit using memguard doesn't make this a memguard issue.

[awnumar]

For now, a note in the documentation and possibly a slightly improved error message is all I'm changing.

In the future we can look into using a soft pointer to the Buffer itself for the DestroyAll function, for the purposes of using a finaliser. This would get rid of the LockedBuffers function, but I'm not sure that it's needed anyways.

[awnumar]

To anyone reading this issue in the future, finalisers were added in #27, and merged in v0.12.0.