ronantakizawa/codereview-bench huggingface dataset:
Repository: opencontainers/runc
File: libcontainer/dmz/_dmz.c (C)

Code before:
#ifdef RUNC_USE_STDLIB
#  include <unistd.h>
#  include <stdio.h>
#  include <string.h>
#else
#  include "xstat.h"
#  include "nolibc/nolibc.h"
#endif

extern char **environ;

int main(int argc, char **argv)
{
	if (argc < 1)
		return 127;
	int r = execve(argv[0], argv, environ);
	if (r) {
		char *prefix = "exec ";
		int err_len = strlen(prefix) + strlen(argv[0]) + 1;
		char err[err_len];

		strcpy(err, prefix);
		strcpy(err + strlen(prefix), argv[0]);
		err[err_len - 1] = '\0';

		perror(err);
	}
	return r;
}


Review comment (suggestion):
Wouldn't something like this be simpler?

```suggestion
		char err[5 + PATH_MAX] = "exec "; // "exec " + argv[0]
		strlcat(err, argv[0], sizeof(err));
		err[sizeof(err) - 1] = '\0';
		perror(err);
```

No dynamic memory management, no VFAs, and no error-prone pointer maths.

Unfortunately, for some reason `strlcat` has issues even though it is defined in `nolibc` (as is `strlen`)...


```
make[1]: Entering directory '/home/cyphar/src/github.com/opencontainers/runc/libcontainer/dmz'
gcc -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib -lgcc -static -o binary/runc-dmz _dmz.c
/usr/lib64/gcc/x86_64-suse-linux/13/../../../../x86_64-suse-linux/bin/ld: /tmp/ccIfhSHD.o: in function `main':
_dmz.c:(.text.startup+0x5e): undefined reference to `strlen'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:18: binary/runc-dmz] Error 1
```

Code after:
#ifdef RUNC_USE_STDLIB
#  include <linux/limits.h>
#  include <stdio.h>
#  include <string.h>
#  include <unistd.h>
#else
#  include "xstat.h"
#  include "nolibc/nolibc.h"
#endif

extern char **environ;

int main(int argc, char **argv)
{
	if (argc < 1)
		return 127;
	int ret = execve(argv[0], argv, environ);
	if (ret) {
		/* NOTE: This error message format MUST match Go's format. */
		char err_msg[5 + PATH_MAX + 1] = "exec ";	// "exec " + argv[0] + '\0'
		strncat(err_msg, argv[0], PATH_MAX);
		err_msg[sizeof(err_msg) - 1] = '\0';

		perror(err_msg);
	}
	return ret;
}
Repository: opencontainers/runc
File: libcontainer/dmz/_dmz.c (C)

Code before:
#ifdef RUNC_USE_STDLIB
#  include <linux/limits.h>
#  include <stdio.h>
#  include <string.h>
#  include <unistd.h>
#else
#  include "xstat.h"
#  include "nolibc/nolibc.h"
#endif

extern char **environ;

int main(int argc, char **argv)
{
	if (argc < 1)
		return 127;
	int ret = execve(argv[0], argv, environ);
	if (ret) {
		char err_msg[5 + PATH_MAX] = "exec ";	// "exec " + argv[0]

		strcpy(err_msg + strlen(err_msg), argv[0]);
		err_msg[sizeof(err_msg) - 1] = '\0';

		perror(err_msg);
	}
	return ret;
}


Review comment (suggestion):
Needs to be `strlcpy`. `argv[0]` is attacker-controlled.

```suggestion
		strlcpy(err_msg + strlen(err_msg), argv[0], PATH_MAX);
```

Code after:
#ifdef RUNC_USE_STDLIB
#  include <linux/limits.h>
#  include <stdio.h>
#  include <string.h>
#  include <unistd.h>
#else
#  include "xstat.h"
#  include "nolibc/nolibc.h"
#endif

extern char **environ;

int main(int argc, char **argv)
{
	if (argc < 1)
		return 127;
	int ret = execve(argv[0], argv, environ);
	if (ret) {
		/* NOTE: This error message format MUST match Go's format. */
		char err_msg[5 + PATH_MAX + 1] = "exec ";	// "exec " + argv[0] + '\0'
		strncat(err_msg, argv[0], PATH_MAX);
		err_msg[sizeof(err_msg) - 1] = '\0';

		perror(err_msg);
	}
	return ret;
}
Repository: opencontainers/runc
File: libcontainer/dmz/_dmz.c (C)

Code before:
#ifdef RUNC_USE_STDLIB
#  include <linux/limits.h>
#  include <stdio.h>
#  include <string.h>
#  include <unistd.h>
#else
#  include "xstat.h"
#  include "nolibc/nolibc.h"
#endif

extern char **environ;

int main(int argc, char **argv)
{
	if (argc < 1)
		return 127;
	int ret = execve(argv[0], argv, environ);
	if (ret) {
		char err_msg[5 + PATH_MAX] = "exec ";	// "exec " + argv[0]

		strlcpy(err_msg + strlen(err_msg), argv[0], PATH_MAX);
		err_msg[sizeof(err_msg) - 1] = '\0';

		perror(err_msg);
	}
	return ret;
}


Review comment (suggestion):
```suggestion
		/* NOTE: This error message format MUST match Go's format. */
		char err_msg[5 + PATH_MAX] = "exec ";	// "exec " + argv[0]
```

Code after:
#ifdef RUNC_USE_STDLIB
#  include <linux/limits.h>
#  include <stdio.h>
#  include <string.h>
#  include <unistd.h>
#else
#  include "xstat.h"
#  include "nolibc/nolibc.h"
#endif

extern char **environ;

int main(int argc, char **argv)
{
	if (argc < 1)
		return 127;
	int ret = execve(argv[0], argv, environ);
	if (ret) {
		/* NOTE: This error message format MUST match Go's format. */
		char err_msg[5 + PATH_MAX + 1] = "exec ";	// "exec " + argv[0] + '\0'
		strncat(err_msg, argv[0], PATH_MAX);
		err_msg[sizeof(err_msg) - 1] = '\0';

		perror(err_msg);
	}
	return ret;
}
Repository: opencontainers/runc
File: libcontainer/dmz/_dmz.c (C)

Code before:
#ifdef RUNC_USE_STDLIB
#  include <unistd.h>
#else
#  include "xstat.h"
#  include "nolibc/nolibc.h"
#endif

extern char **environ;

int main(int argc, char **argv)
{
	if (argc < 1)
		return 127;
	return execve(argv[0], argv, environ);
}


Review comment (suggestion):
Ah, I didn't notice you switched to `strncat`. In that case, the length should really be `sizeof(err_msg)`.

Code after:
#ifdef RUNC_USE_STDLIB
#  include <linux/limits.h>
#  include <stdio.h>
#  include <string.h>
#  include <unistd.h>
#else
#  include "xstat.h"
#  include "nolibc/nolibc.h"
#endif

extern char **environ;

int main(int argc, char **argv)
{
	if (argc < 1)
		return 127;
	int ret = execve(argv[0], argv, environ);
	if (ret) {
		/* NOTE: This error message format MUST match Go's format. */
		char err_msg[5 + PATH_MAX + 1] = "exec ";	// "exec " + argv[0] + '\0'
		strncat(err_msg, argv[0], PATH_MAX);
		err_msg[sizeof(err_msg) - 1] = '\0';

		perror(err_msg);
	}
	return ret;
}
Repository: opencontainers/runc
File: libcontainer/sync_unix.go (Go)

Code before:
package libcontainer

import (
	"fmt"
	"io"
	"os"

	"golang.org/x/sys/unix"
)

// syncSocket is a wrapper around a SOCK_SEQPACKET socket, providing
// packet-oriented methods. This is needed because SOCK_SEQPACKET does not
// allow for partial reads, but the Go stdlib treats it as a streamable source,
// which ends up making things like json.Decoder hang forever if the packet is
// bigger than the internal read buffer.
type syncSocket struct {
	f         *os.File
	procReady bool
}

func newSyncSocket(f *os.File) *syncSocket {
	return &syncSocket{f: f}
}

func (s *syncSocket) File() *os.File {
	return s.f
}

func (s *syncSocket) Close() error {
	return s.f.Close()
}

func (s *syncSocket) WritePacket(b []byte) (int, error) {
	return s.f.Write(b)
}

func (s *syncSocket) ReadPacket() ([]byte, error) {
	size, _, err := unix.Recvfrom(int(s.f.Fd()), nil, unix.MSG_TRUNC|unix.MSG_PEEK)
	if err != nil {
		return nil, fmt.Errorf("fetch packet length from socket: %w", err)
	}
	// We will only get a zero size if the socket has been closed from the
	// other end (otherwise recvfrom(2) will block until a packet is ready). In

Review comment (suggestion):
I don't like that this is put inside `syncSocket` because:

1. `procReady` only makes sense for one of our pipes, but we have multiple.
2. With this patch, `procReady` means two different things for `runc init` and the parent runc process. Combining the logic makes it far more confusing.

That being said, maybe having a `closed bool` inside `syncSocket` that is only be used for `runc init` might make some sense (since we are effectively tracking whether the pipe is closed) though I don't like that access to the bool isn't protected by a mutex (then again, I suspect we wouldn't concurrently close it so :shrug:.)

Code after:
package libcontainer

import (
	"fmt"
	"io"
	"os"
	"sync/atomic"

	"golang.org/x/sys/unix"
)

// syncSocket is a wrapper around a SOCK_SEQPACKET socket, providing
// packet-oriented methods. This is needed because SOCK_SEQPACKET does not
// allow for partial reads, but the Go stdlib treats it as a streamable source,
// which ends up making things like json.Decoder hang forever if the packet is
// bigger than the internal read buffer.
type syncSocket struct {
	f      *os.File
	closed atomic.Bool
}

func newSyncSocket(f *os.File) *syncSocket {
	return &syncSocket{f: f}
}

func (s *syncSocket) File() *os.File {
	return s.f
}

func (s *syncSocket) Close() error {
	// Even with errors from Close(), we have to assume the pipe was closed.
	s.closed.Store(true)
	return s.f.Close()
}

func (s *syncSocket) isClosed() bool {
	return s.closed.Load()
}

func (s *syncSocket) WritePacket(b []byte) (int, error) {
	return s.f.Write(b)
}
Repository: opencontainers/runc
File: libcontainer/sync_unix.go (Go)

Code before:
package libcontainer

import (
	"fmt"
	"io"
	"os"
	"sync"

	"golang.org/x/sys/unix"
)

// syncSocket is a wrapper around a SOCK_SEQPACKET socket, providing
// packet-oriented methods. This is needed because SOCK_SEQPACKET does not
// allow for partial reads, but the Go stdlib treats it as a streamable source,
// which ends up making things like json.Decoder hang forever if the packet is
// bigger than the internal read buffer.
type syncSocket struct {
	f      *os.File
	closed bool
	m      sync.Mutex
}

func newSyncSocket(f *os.File) *syncSocket {
	return &syncSocket{f: f}
}

func (s *syncSocket) File() *os.File {
	return s.f
}

func (s *syncSocket) Close() error {
	s.m.Lock()
	defer s.m.Unlock()

	// We can set closed to true here anyway without checking the error.
	s.closed = true
	return s.f.Close()
}

func (s *syncSocket) IsNomalClosed() bool {
	s.m.Lock()
	defer s.m.Unlock()

	// We need to know that whether the sync socket is normally closed or not.
	// If the sync socket is closed by accident, we don't care about it.

Review comment (suggestion):
```suggestion
	closed atomic.Bool
```

This might make things a bit nicer...

Code after:
package libcontainer

import (
	"fmt"
	"io"
	"os"
	"sync/atomic"

	"golang.org/x/sys/unix"
)

// syncSocket is a wrapper around a SOCK_SEQPACKET socket, providing
// packet-oriented methods. This is needed because SOCK_SEQPACKET does not
// allow for partial reads, but the Go stdlib treats it as a streamable source,
// which ends up making things like json.Decoder hang forever if the packet is
// bigger than the internal read buffer.
type syncSocket struct {
	f      *os.File
	closed atomic.Bool
}

func newSyncSocket(f *os.File) *syncSocket {
	return &syncSocket{f: f}
}

func (s *syncSocket) File() *os.File {
	return s.f
}

func (s *syncSocket) Close() error {
	// Even with errors from Close(), we have to assume the pipe was closed.
	s.closed.Store(true)
	return s.f.Close()
}

func (s *syncSocket) isClosed() bool {
	return s.closed.Load()
}

func (s *syncSocket) WritePacket(b []byte) (int, error) {
	return s.f.Write(b)
}

func (s *syncSocket) ReadPacket() ([]byte, error) {
	size, _, err := unix.Recvfrom(int(s.f.Fd()), nil, unix.MSG_TRUNC|unix.MSG_PEEK)
Repository: opencontainers/runc
File: libcontainer/setns_init_linux.go (Go)

Code before:
}

func (l *linuxSetnsInit) Init() error {
	if !l.config.Config.NoNewKeyring {
		if err := selinux.SetKeyLabel(l.config.ProcessLabel); err != nil {
			return err
		}
		defer selinux.SetKeyLabel("") //nolint: errcheck
		// Do not inherit the parent's session keyring.
		if _, err := keys.JoinSessionKeyring(l.getSessionRingName()); err != nil {
			// Same justification as in standart_init_linux.go as to why we
			// don't bail on ENOSYS.
			//
			// TODO(cyphar): And we should have logging here too.
			if !errors.Is(err, unix.ENOSYS) {
				return fmt.Errorf("unable to join session keyring: %w", err)
			}
		}
	}

	// Clean the RLIMIT_NOFILE cache in go runtime.
	// Issue: https://github.com/opencontainers/runc/issues/4195
	if containsRlimit(l.config.Rlimits, unix.RLIMIT_NOFILE) {
		system.ClearRlimitNofileCache()
	}

	if l.config.CreateConsole {
		if err := setupConsole(l.consoleSocket, l.config, false); err != nil {
			return err
		}
		if err := system.Setctty(); err != nil {
			return err
		}
	}
	if l.pidfdSocket != nil {
		if err := setupPidfd(l.pidfdSocket, "setns"); err != nil {
			return fmt.Errorf("failed to setup pidfd: %w", err)
		}
	}
	if l.config.NoNewPrivileges {
		if err := unix.Prctl(unix.PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); err != nil {
			return err
		}
	}
	if l.config.Config.Umask != nil {
		unix.Umask(int(*l.config.Config.Umask))
	}

	if l.config.Config.Scheduler != nil {
		if err := setupScheduler(l.config.Config); err != nil {
			return err

Review comment (performance):
Can you explain why we need the `containsRlimit`? Would be easier to always remove the cache as we don't really need it in runc.

Code after:
}

func (l *linuxSetnsInit) Init() error {
	if !l.config.Config.NoNewKeyring {
		if err := selinux.SetKeyLabel(l.config.ProcessLabel); err != nil {
			return err
		}
		defer selinux.SetKeyLabel("") //nolint: errcheck
		// Do not inherit the parent's session keyring.
		if _, err := keys.JoinSessionKeyring(l.getSessionRingName()); err != nil {
			// Same justification as in standart_init_linux.go as to why we
			// don't bail on ENOSYS.
			//
			// TODO(cyphar): And we should have logging here too.
			if !errors.Is(err, unix.ENOSYS) {
				return fmt.Errorf("unable to join session keyring: %w", err)
			}
		}
	}

	if l.config.CreateConsole {
		if err := setupConsole(l.consoleSocket, l.config, false); err != nil {
			return err
		}
		if err := system.Setctty(); err != nil {
			return err
		}
	}
	if l.pidfdSocket != nil {
		if err := setupPidfd(l.pidfdSocket, "setns"); err != nil {
			return fmt.Errorf("failed to setup pidfd: %w", err)
		}
	}
	if l.config.NoNewPrivileges {
		if err := unix.Prctl(unix.PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0); err != nil {
			return err
		}
	}
	if l.config.Config.Umask != nil {
		unix.Umask(int(*l.config.Config.Umask))
	}

	if l.config.Config.Scheduler != nil {
		if err := setupScheduler(l.config.Config); err != nil {
			return err
		}
	}

	// Tell our parent that we're ready to exec. This must be done before the
	// Seccomp rules have been applied, because we need to be able to read and
	// write to a socket.
Repository: opencontainers/runc
File: libcontainer/system/rlimit_linux_go122.go (Go)

Code before:
//go:build !go1.23

package system

import (
	"sync/atomic"
	"syscall"
	_ "unsafe" // Needed for go:linkname to work.
)

//go:linkname syscallOrigRlimitNofile syscall.origRlimitNofile
var syscallOrigRlimitNofile atomic.Pointer[syscall.Rlimit]

// ClearRlimitNofileCache clears go runtime's nofile rlimit cache.
// The argument is process RLIMIT_NOFILE values.
func ClearRlimitNofileCache(_ *syscall.Rlimit) {
	// As reported in issue #4195, the new version of go runtime(since 1.19)
	// will cache rlimit-nofile. Before executing execve, the rlimit-nofile
	// of the process will be restored with the cache. In runc, this will
	// cause the rlimit-nofile setting by the parent process for the container
	// to become invalid. It can be solved by clearing this cache. But
	// unfortunately, go stdlib doesn't provide such function, so we need to
	// link to the private var `origRlimitNofile` in package syscall to hack.
	syscallOrigRlimitNofile.Store(nil)
}


Review comment (performance):
When I was choosing go linkname hack, I really want to add a comment like this:
FIXME: Once golang has a public API to clear the nofile rlimit cache, we should remove this hack and switch to the official API.
But it seems like there is no chance to add this specific API in golang, so it's no need to add this similar comment, but maybe we can add a comment to inform other contributors that we should not work around for the `hall of shame` anymore. For example:
// Note: we should remove this hack once runc doesn't support 1.22 anymore.

Code after:
//go:build !go1.23

// TODO: remove this file once go 1.22 is no longer supported.

package system

import (
	"sync/atomic"
	"syscall"
	_ "unsafe" // Needed for go:linkname to work.
)

//go:linkname syscallOrigRlimitNofile syscall.origRlimitNofile
var syscallOrigRlimitNofile atomic.Pointer[syscall.Rlimit]

// ClearRlimitNofileCache clears go runtime's nofile rlimit cache.
// The argument is process RLIMIT_NOFILE values.
func ClearRlimitNofileCache(_ *syscall.Rlimit) {
	// As reported in issue #4195, the new version of go runtime(since 1.19)
	// will cache rlimit-nofile. Before executing execve, the rlimit-nofile
	// of the process will be restored with the cache. In runc, this will
	// cause the rlimit-nofile setting by the parent process for the container
	// to become invalid. It can be solved by clearing this cache. But
	// unfortunately, go stdlib doesn't provide such function, so we need to
	// link to the private var `origRlimitNofile` in package syscall to hack.
	syscallOrigRlimitNofile.Store(nil)
}
Repository: opencontainers/runc
File: libcontainer/cgroups/fs/fs.go (Go)

Code before:

func (m *Manager) Apply(pid int) (err error) {
	m.mu.Lock()
	defer m.mu.Unlock()

	c := m.cgroups

	for _, sys := range subsystems {
		name := sys.Name()
		p, ok := m.paths[name]
		if !ok {
			continue
		}

		if err := sys.Apply(p, c.Resources, pid); err != nil {
			// In the case of rootless (including euid=0 in userns), where an
			// explicit cgroup path hasn't been set, we don't bail on error in
			// case of permission problems here, but do delete the path from
			// the m.paths map, since it is either non-existent and could not
			// be created, or the pid could not be added to it.
			//
			// Cases where limits for the subsystem have been set are handled
			// later by Set, which fails with a friendly error (see
			// if path == "" in Set).
			if isIgnorableError(c.Rootless, err) && c.Path == "" {
				delete(m.paths, name)
				continue
			}
			return err
		}

	}
	return nil
}

func (m *Manager) Destroy() error {
	m.mu.Lock()
	defer m.mu.Unlock()
	return cgroups.RemovePaths(m.paths)
}

func (m *Manager) Path(subsys string) string {
	m.mu.Lock()
	defer m.mu.Unlock()
	return m.paths[subsys]
}

func (m *Manager) GetStats() (*cgroups.Stats, error) {
	m.mu.Lock()
	defer m.mu.Unlock()
	stats := cgroups.NewStats()

Review comment (suggestion):
Do you think this is too strict? Maybe we should add a condition here:
```
if name == "devices" {
    retErr = cgroups.ErrRootless
}
```

Code after:

func (m *Manager) Apply(pid int) (retErr error) {
	m.mu.Lock()
	defer m.mu.Unlock()

	c := m.cgroups

	for _, sys := range subsystems {
		name := sys.Name()
		p, ok := m.paths[name]
		if !ok {
			continue
		}

		if err := sys.Apply(p, c.Resources, pid); err != nil {
			// In the case of rootless (including euid=0 in userns), where an
			// explicit cgroup path hasn't been set, we don't bail on error in
			// case of permission problems here, but do delete the path from
			// the m.paths map, since it is either non-existent and could not
			// be created, or the pid could not be added to it.
			//
			// Cases where limits for the subsystem have been set are handled
			// later by Set, which fails with a friendly error (see
			// if path == "" in Set).
			if isIgnorableError(c.Rootless, err) && c.Path == "" {
				retErr = cgroups.ErrRootless
				delete(m.paths, name)
				continue
			}
			return err
		}

	}
	return retErr
}

func (m *Manager) Destroy() error {
	m.mu.Lock()
	defer m.mu.Unlock()
	return cgroups.RemovePaths(m.paths)
}

func (m *Manager) Path(subsys string) string {
	m.mu.Lock()
	defer m.mu.Unlock()
	return m.paths[subsys]
}

func (m *Manager) GetStats() (*cgroups.Stats, error) {
	m.mu.Lock()
	defer m.mu.Unlock()
Repository: opencontainers/runc
File: libcontainer/utils/utils.go (Go)

Code before:
	case strings.HasPrefix(path, root+"/"):
		path = strings.TrimPrefix(path, root+"/")
	}
	return CleanPath("/" + path)
}

// SearchLabels searches through a list of key=value pairs for a given key,
// returning its value, and the binary flag telling whether the key exist.
func SearchLabels(labels []string, key string) (string, bool) {
	key += "="
	for _, s := range labels {
		if strings.HasPrefix(s, key) {
			return s[len(key):], true
		}
	}
	return "", false
}

// Annotations returns the bundle path and user defined annotations from the
// libcontainer state.  We need to remove the bundle because that is a label
// added by libcontainer.
func Annotations(labels []string) (bundle string, userAnnotations map[string]string) {
	userAnnotations = make(map[string]string)
	for _, l := range labels {
		name, value, isValidAnnotation := strings.Cut(l, "=")
		if !isValidAnnotation {
			continue
		}
		if name == "bundle" {
			bundle = value
		} else {
			userAnnotations[name] = value
		}
	}
	return
}


Review comment (suggestion):
Here as well;

```go
name, value, ok := strings.Cut(l, "=")
if !ok {
```

Code after:
	case strings.HasPrefix(path, root+"/"):
		path = strings.TrimPrefix(path, root+"/")
	}
	return CleanPath("/" + path)
}

// SearchLabels searches through a list of key=value pairs for a given key,
// returning its value, and the binary flag telling whether the key exist.
func SearchLabels(labels []string, key string) (string, bool) {
	key += "="
	for _, s := range labels {
		if strings.HasPrefix(s, key) {
			return s[len(key):], true
		}
	}
	return "", false
}

// Annotations returns the bundle path and user defined annotations from the
// libcontainer state.  We need to remove the bundle because that is a label
// added by libcontainer.
func Annotations(labels []string) (bundle string, userAnnotations map[string]string) {
	userAnnotations = make(map[string]string)
	for _, l := range labels {
		name, value, ok := strings.Cut(l, "=")
		if !ok {
			continue
		}
		if name == "bundle" {
			bundle = value
		} else {
			userAnnotations[name] = value
		}
	}
	return
}