Allow and document bug ids in gVisor codebase.

PiperOrigin-RevId: 245818639
Change-Id: I03703ef0fb9b6675955637b9fe2776204c545789

CONTRIBUTING.md

@@ -126,6 +126,13 @@ change.
 When approved, the change will be submitted by a team member and automatically
 merged into the repository.
 
+### Bug IDs
+
+Some TODOs and NOTEs sprinkled throughout the code have associated IDs of the
+form b/1234. These correspond to bugs in our internal bug tracker. Eventually
+these bugs will be moved to the GitHub Issues, but until then they can simply be
+ignored.
+
 ### The small print
 
 Contributions made by corporations are covered by a different agreement than the

pkg/cpuid/cpuid_test.go

@@ -78,7 +78,7 @@ func TestTakeFeatureIntersection(t *testing.T) {
 	}
 }
 
-// TODO: Run this test on a very old platform, and make sure more
+// TODO(b/73346484): Run this test on a very old platform, and make sure more
 // bits are enabled than just FPU and PAE. This test currently may not detect
 // if HostFeatureSet gives back junk bits.
 func TestHostFeatureSet(t *testing.T) {

pkg/dhcp/client.go

@@ -120,7 +120,7 @@ func (c *Client) Config() Config {
 // If the server sets a lease limit a timer is set to automatically
 // renew it.
 func (c *Client) Request(ctx context.Context, requestedAddr tcpip.Address) (cfg Config, reterr error) {
-	// TODO: remove calls to {Add,Remove}Address when they're no
+	// TODO(b/127321246): remove calls to {Add,Remove}Address when they're no
 	// longer required to send and receive broadcast.
 	if err := c.stack.AddAddressWithOptions(c.nicid, ipv4.ProtocolNumber, tcpipHeader.IPv4Any, stack.NeverPrimaryEndpoint); err != nil && err != tcpip.ErrDuplicateAddress {
 		return Config{}, fmt.Errorf("dhcp: AddAddressWithOptions(): %s", err)

pkg/log/glog.go

@@ -144,7 +144,7 @@ func (g GoogleEmitter) Emit(level Level, timestamp time.Time, format string, arg
 	b.writeAll(pid)
 	b.write(' ')
 
-	// FIXME: The caller, fabricated. This really sucks, but it
+	// FIXME(b/73383460): The caller, fabricated. This really sucks, but it
 	// is unacceptable to put runtime.Callers() in the hot path.
 	b.writeAll(caller)
 	b.write(']')

pkg/metric/metric.go

@@ -44,8 +44,8 @@ var (
 //
 // Metrics are not saved across save/restore and thus reset to zero on restore.
 //
-// TODO: Support non-cumulative metrics.
-// TODO: Support metric fields.
+// TODO(b/67298402): Support non-cumulative metrics.
+// TODO(b/67298427): Support metric fields.
 //
 type Uint64Metric struct {
 	// value is the actual value of the metric. It must be accessed

pkg/segment/set.go

@@ -1270,7 +1270,7 @@ func segmentAfterPosition(n *node, i int) Iterator {
 }
 
 func zeroValueSlice(slice []Value) {
-	// TODO: check if Go is actually smart enough to optimize a
+	// TODO(jamieliu): check if Go is actually smart enough to optimize a
 	// ClearValue that assigns nil to a memset here
 	for i := range slice {
 		Functions{}.ClearValue(&slice[i])

pkg/segment/test/set_functions.go

@@ -15,7 +15,7 @@
 package segment
 
 // Basic numeric constants that we define because the math package doesn't.
-// TODO: These should be Math.MaxInt64/MinInt64?
+// TODO(nlacasse): These should be Math.MaxInt64/MinInt64?
 const (
 	maxInt = int(^uint(0) >> 1)
 	minInt = -maxInt - 1

pkg/sentry/arch/arch.go

@@ -53,7 +53,7 @@ type FloatingPointData byte
 
 // Context provides architecture-dependent information for a specific thread.
 //
-// NOTE: Currently we use uintptr here to refer to a generic native
+// NOTE(b/34169503): Currently we use uintptr here to refer to a generic native
 // register value. While this will work for the foreseeable future, it isn't
 // strictly correct. We may want to create some abstraction that makes this
 // more clear or enables us to store values of arbitrary widths. This is

pkg/sentry/arch/arch_amd64.go

@@ -305,7 +305,7 @@ func (c *context64) PtracePeekUser(addr uintptr) (interface{}, error) {
 		buf := binary.Marshal(nil, usermem.ByteOrder, c.ptraceGetRegs())
 		return c.Native(uintptr(usermem.ByteOrder.Uint64(buf[addr:]))), nil
 	}
-	// TODO: debug registers
+	// TODO(b/34088053): debug registers
 	return c.Native(0), nil
 }
 
@@ -320,6 +320,6 @@ func (c *context64) PtracePokeUser(addr, data uintptr) error {
 		_, err := c.PtraceSetRegs(bytes.NewBuffer(buf))
 		return err
 	}
-	// TODO: debug registers
+	// TODO(b/34088053): debug registers
 	return nil
 }

pkg/sentry/arch/arch_x86.go

@@ -306,7 +306,7 @@ func (s *State) ptraceGetRegs() syscall.PtraceRegs {
 	// FS/GS_TLS_SEL when fs_base/gs_base is a 64-bit value. (We do the
 	// same in PtraceSetRegs.)
 	//
-	// TODO: Remove this fixup since newer Linux
+	// TODO(gvisor.dev/issue/168): Remove this fixup since newer Linux
 	// doesn't have this behavior anymore.
 	if regs.Fs == 0 && regs.Fs_base <= 0xffffffff {
 		regs.Fs = _FS_TLS_SEL

pkg/sentry/arch/signal_amd64.go

@@ -319,7 +319,7 @@ func (c *context64) NewSignalStack() NativeSignalStack {
 // From Linux 'arch/x86/include/uapi/asm/sigcontext.h' the following is the
 // size of the magic cookie at the end of the xsave frame.
 //
-// NOTE: Currently we don't actually populate the fpstate
+// NOTE(b/33003106#comment11): Currently we don't actually populate the fpstate
 // on the signal stack.
 const _FP_XSTATE_MAGIC2_SIZE = 4
 
@@ -392,7 +392,7 @@ func (c *context64) SignalSetup(st *Stack, act *SignalAct, info *SignalInfo, alt
 		Sigset: sigset,
 	}
 
-	// TODO: Set SignalContext64.Err, Trapno, and Cr2
+	// TODO(gvisor.dev/issue/159): Set SignalContext64.Err, Trapno, and Cr2
 	// based on the fault that caused the signal. For now, leave Err and
 	// Trapno unset and assume CR2 == info.Addr() for SIGSEGVs and
 	// SIGBUSes.
@@ -505,7 +505,7 @@ func (c *context64) SignalRestore(st *Stack, rt bool) (linux.SignalSet, SignalSt
 	l := len(c.sigFPState)
 	if l > 0 {
 		c.x86FPState = c.sigFPState[l-1]
-		// NOTE: State save requires that any slice
+		// NOTE(cl/133042258): State save requires that any slice
 		// elements from '[len:cap]' to be zero value.
 		c.sigFPState[l-1] = nil
 		c.sigFPState = c.sigFPState[0 : l-1]

pkg/sentry/arch/stack.go

@@ -97,7 +97,7 @@ func (s *Stack) Push(vals ...interface{}) (usermem.Addr, error) {
 		if c < 0 {
 			return 0, fmt.Errorf("bad binary.Size for %T", v)
 		}
-		// TODO: Use a real context.Context.
+		// TODO(b/38173783): Use a real context.Context.
 		n, err := usermem.CopyObjectOut(context.Background(), s.IO, s.Bottom-usermem.Addr(c), norm, usermem.IOOpts{})
 		if err != nil || c != n {
 			return 0, err
@@ -121,11 +121,11 @@ func (s *Stack) Pop(vals ...interface{}) (usermem.Addr, error) {
 		var err error
 		if isVaddr {
 			value := s.Arch.Native(uintptr(0))
-			// TODO: Use a real context.Context.
+			// TODO(b/38173783): Use a real context.Context.
 			n, err = usermem.CopyObjectIn(context.Background(), s.IO, s.Bottom, value, usermem.IOOpts{})
 			*vaddr = usermem.Addr(s.Arch.Value(value))
 		} else {
-			// TODO: Use a real context.Context.
+			// TODO(b/38173783): Use a real context.Context.
 			n, err = usermem.CopyObjectIn(context.Background(), s.IO, s.Bottom, v, usermem.IOOpts{})
 		}
 		if err != nil {

pkg/sentry/context/context.go

@@ -114,7 +114,7 @@ var bgContext = &logContext{Logger: log.Log()}
 // Background returns an empty context using the default logger.
 //
 // Users should be wary of using a Background context. Please tag any use with
-// FIXME and a note to remove this use.
+// FIXME(b/38173783) and a note to remove this use.
 //
 // Generally, one should use the Task as their context when available, or avoid
 // having to use a context in places where a Task is unavailable.

pkg/sentry/control/proc.go

@@ -261,7 +261,7 @@ func (proc *Proc) Ps(args *PsArgs, out *string) error {
 }
 
 // Process contains information about a single process in a Sandbox.
-// TODO: Implement TTY field.
+// TODO(b/117881927): Implement TTY field.
 type Process struct {
 	UID auth.KUID       `json:"uid"`
 	PID kernel.ThreadID `json:"pid"`

pkg/sentry/fs/README.md

@@ -59,7 +59,7 @@ two categories:
 
 The first is always necessary to save and restore. An application may never have
 any open file descriptors, but across save and restore it should see a coherent
-view of any mount namespace. NOTE: Currently only one "initial"
+view of any mount namespace. NOTE(b/63601033): Currently only one "initial"
 mount namespace is supported.
 
 The second is so that system calls across save and restore are coherent with

pkg/sentry/fs/ashmem/area.go

@@ -240,7 +240,7 @@ func (a *Area) Ioctl(ctx context.Context, io usermem.IO, args arch.SyscallArgume
 			return 0, syserror.EINVAL
 		}
 
-		// TODO: If personality flag
+		// TODO(b/30946773,gvisor.dev/issue/153): If personality flag
 		// READ_IMPLIES_EXEC is set, set PROT_EXEC if PORT_READ is set.
 
 		a.perms = perms
@@ -290,7 +290,7 @@ func (a *Area) pinOperation(pin linux.AshmemPin, op uint32) (uintptr, error) {
 		return linux.AshmemNotPurged, nil
 
 	case linux.AshmemUnpinIoctl:
-		// TODO: Implement purge on unpin.
+		// TODO(b/30946773): Implement purge on unpin.
 		a.pb.UnpinRange(r)
 		return 0, nil
 

pkg/sentry/fs/binder/binder.go

@@ -69,7 +69,7 @@ func NewDevice(ctx context.Context, owner fs.FileOwner, fp fs.FilePermissions) *
 
 // GetFile implements fs.InodeOperations.GetFile.
 //
-// TODO: Add functionality to GetFile: Additional fields will be
+// TODO(b/30946773): Add functionality to GetFile: Additional fields will be
 // needed in the Device structure, initialize them here. Also, Device will need
 // to keep track of the created Procs in order to implement BINDER_READ_WRITE
 // ioctl.
@@ -133,7 +133,7 @@ func (bp *Proc) Write(ctx context.Context, file *fs.File, src usermem.IOSequence
 
 // Flush implements fs.FileOperations.Flush.
 //
-// TODO: Implement.
+// TODO(b/30946773): Implement.
 func (bp *Proc) Flush(ctx context.Context, file *fs.File) error {
 	return nil
 }
@@ -149,7 +149,7 @@ func (bp *Proc) ConfigureMMap(ctx context.Context, file *fs.File, opts *memmap.M
 	}
 	opts.MaxPerms.Write = false
 
-	// TODO: Binder sets VM_DONTCOPY, preventing the created vma
+	// TODO(b/30946773): Binder sets VM_DONTCOPY, preventing the created vma
 	// from being copied across fork(), but we don't support this yet. As
 	// a result, MMs containing a Binder mapping cannot be forked (MM.Fork will
 	// fail when AddMapping returns EBUSY).
@@ -159,7 +159,7 @@ func (bp *Proc) ConfigureMMap(ctx context.Context, file *fs.File, opts *memmap.M
 
 // Ioctl implements fs.FileOperations.Ioctl.
 //
-// TODO: Implement.
+// TODO(b/30946773): Implement.
 func (bp *Proc) Ioctl(ctx context.Context, io usermem.IO, args arch.SyscallArguments) (uintptr, error) {
 	// Switch on ioctl request.
 	switch uint32(args[1].Int()) {
@@ -173,22 +173,22 @@ func (bp *Proc) Ioctl(ctx context.Context, io usermem.IO, args arch.SyscallArgum
 		})
 		return 0, err
 	case linux.BinderWriteReadIoctl:
-		// TODO: Implement.
+		// TODO(b/30946773): Implement.
 		fallthrough
 	case linux.BinderSetIdleTimeoutIoctl:
-		// TODO: Implement.
+		// TODO(b/30946773): Implement.
 		fallthrough
 	case linux.BinderSetMaxThreadsIoctl:
-		// TODO: Implement.
+		// TODO(b/30946773): Implement.
 		fallthrough
 	case linux.BinderSetIdlePriorityIoctl:
-		// TODO: Implement.
+		// TODO(b/30946773): Implement.
 		fallthrough
 	case linux.BinderSetContextMgrIoctl:
-		// TODO: Implement.
+		// TODO(b/30946773): Implement.
 		fallthrough
 	case linux.BinderThreadExitIoctl:
-		// TODO: Implement.
+		// TODO(b/30946773): Implement.
 		return 0, syserror.ENOSYS
 	default:
 		// Ioctls irrelevant to Binder.
@@ -228,7 +228,7 @@ func (bp *Proc) CopyMapping(ctx context.Context, ms memmap.MappingSpace, srcAR,
 
 // Translate implements memmap.Mappable.Translate.
 func (bp *Proc) Translate(ctx context.Context, required, optional memmap.MappableRange, at usermem.AccessType) ([]memmap.Translation, error) {
-	// TODO: In addition to the page initially allocated and mapped
+	// TODO(b/30946773): In addition to the page initially allocated and mapped
 	// in AddMapping (Linux: binder_mmap), Binder allocates and maps pages for
 	// each transaction (Linux: binder_ioctl => binder_ioctl_write_read =>
 	// binder_thread_write => binder_transaction => binder_alloc_buf =>

pkg/sentry/fs/dentry.go

@@ -83,7 +83,7 @@ type DirCtx struct {
 	attrs map[string]DentAttr
 
 	// DirCursor is the directory cursor.
-	// TODO: Once Handles are removed this can just live in the
+	// TODO(b/67778717): Once Handles are removed this can just live in the
 	// respective FileOperations implementations and not need to get
 	// plumbed everywhere.
 	DirCursor *string

pkg/sentry/fs/dirent.go

@@ -318,7 +318,7 @@ func (d *Dirent) SyncAll(ctx context.Context) {
 
 	// There is nothing to sync for a read-only filesystem.
 	if !d.Inode.MountSource.Flags.ReadOnly {
-		// FIXME: This should be a mount traversal, not a
+		// FIXME(b/34856369): This should be a mount traversal, not a
 		// Dirent traversal, because some Inodes that need to be synced
 		// may no longer be reachable by name (after sys_unlink).
 		//
@@ -1506,7 +1506,7 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 	}
 
 	// Are we frozen?
-	// TODO: Is this the right errno?
+	// TODO(jamieliu): Is this the right errno?
 	if oldParent.frozen && !oldParent.Inode.IsVirtual() {
 		return syscall.ENOENT
 	}
@@ -1565,7 +1565,7 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 	} else {
 		// Check constraints on the dirent being replaced.
 
-		// NOTE: We don't want to keep replaced alive
+		// NOTE(b/111808347): We don't want to keep replaced alive
 		// across the Rename, so must call DecRef manually (no defer).
 
 		// Check that we can delete replaced.
@@ -1606,7 +1606,7 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 		// Allow the file system to drop extra references on replaced.
 		replaced.dropExtendedReference()
 
-		// NOTE: Keeping a dirent
+		// NOTE(b/31798319,b/31867149,b/31867671): Keeping a dirent
 		// open across renames is currently broken for multiple
 		// reasons, so we flush all references on the replaced node and
 		// its children.

pkg/sentry/fs/file.go

@@ -65,7 +65,7 @@ const FileMaxOffset = math.MaxInt64
 // under a single abortable mutex which also synchronizes lseek(2), read(2),
 // and write(2).
 //
-// FIXME: Split synchronization from cancellation.
+// FIXME(b/38451980): Split synchronization from cancellation.
 //
 // +stateify savable
 type File struct {