Repository: etcd-io/etcd (language: go)

Issue:
Possible missing nil checks
### Bug report criteria

- [X] This bug report is not security related, security issues should be disclosed privately via [etcd maintainers](mailto:etcd-maintainers@googlegroups.com).
- [X] This is not a support request or question, support requests or questions should be raised in the etcd [discussion forums](https://github.com/etcd-io/etcd/discussions).
- [X] You have read the etcd [bug reporting guidelines](https://github.com/etcd-io/etcd/blob/main/Documentation/contributor-guide/reporting_bugs.md).
- [X] Existing open issues along with etcd [frequently asked questions](https://etcd.io/docs/latest/faq) have been checked and this is not a duplicate.

### What happened?

After having been compared to a nil value at backend.go:515, pointer 'b.lg' is passed as implicit 'this' parameter in call to function 'Error' at backend.go:545 or pointer 'b.lg' is passed as implicit 'this' parameter in call to function 'Fatal' at backend.go:532. 
```
if b.lg != nil {
		b.lg.Info(
			"defragmenting",
			zap.String("path", dbp),
			zap.Int64("current-db-size-bytes", size1),
			zap.String("current-db-size", humanize.Bytes(uint64(size1))),
			zap.Int64("current-db-size-in-use-bytes", sizeInUse1),
			zap.String("current-db-size-in-use", humanize.Bytes(uint64(sizeInUse1))),
		)
	}

	defer func() {
		// NOTE: We should exit as soon as possible because that tx
		// might be closed. The inflight request might use invalid
		// tx and then panic as well. The real panic reason might be
		// shadowed by new panic. So, we should fatal here with lock.
		if rerr := recover(); rerr != nil {
			b.lg.Fatal("unexpected panic during defrag", zap.Any("panic", rerr))
		}
	}()

	// Commit/stop and then reset current transactions (including the readTx)
	b.batchTx.unsafeCommit(true)
	b.batchTx.tx = nil

	// gofail: var defragBeforeCopy struct{}
	err = defragdb(b.db, tmpdb, defragLimit)
	if err != nil {
		tmpdb.Close()
		if rmErr := os.RemoveAll(tmpdb.Path()); rmErr != nil {
			b.lg.Error("failed to remove db.tmp after defragmentation completed", zap.Error(rmErr))
		}

		// restore the bbolt transactions if defragmentation fails
		b.batchTx.tx = b.unsafeBegin(true)
		b.readTx.tx = b.unsafeBegin(false)

		return err
	}
```
Please clarify whether verification is required in this case? Do you need to make any changes/corrections to the code?

### What did you expect to happen?

It might be worth adding additional checks for b.lg, if required.

### How can we reproduce it (as minimally and precisely as possible)?

Checking code

### Anything else we need to know?

_No response_

### Etcd version (please run commands below)

Latest

### Etcd configuration (command line flags or environment variables)

<details>



</details>


### Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

<details>



</details>


### Relevant log output

_No response_


Discussion / hints:
We can ensure that `newBackend` (see below) always returns a backend instance with a non-nil [lg](https://github.com/etcd-io/etcd/blob/3cf550d3a47ecee743eaadec1ebd183a0fa88383/server/storage/backend/backend.go#L129). If users pass in a nil lg, then we assign it a `zap.NewNop()`.

Afterwards, we can remove all the ugly nil check.


https://github.com/etcd-io/etcd/blob/3cf550d3a47ecee743eaadec1ebd183a0fa88383/server/storage/backend/backend.go#L185
Hey, I can take this up!

Fix patch:
diff --git a/server/storage/backend/backend.go b/server/storage/backend/backend.go
index 7f5862c8861..275064f083b 100644
--- a/server/storage/backend/backend.go
+++ b/server/storage/backend/backend.go
@@ -28,6 +28,7 @@ import (
 	"go.uber.org/zap"
 
 	bolt "go.etcd.io/bbolt"
+	"go.etcd.io/etcd/client/pkg/v3/verify"
 )
 
 var (
@@ -187,6 +188,11 @@ func newBackend(bcfg BackendConfig) *backend {
 	if boltOpenOptions != nil {
 		*bopts = *boltOpenOptions
 	}
+
+	if bcfg.Logger == nil {
+		bcfg.Logger = zap.NewNop()
+	}
+
 	bopts.InitialMmapSize = bcfg.mmapSize()
 	bopts.FreelistType = bcfg.BackendFreelistType
 	bopts.NoSync = bcfg.UnsafeNoFsync
@@ -458,6 +464,7 @@ func (b *backend) Defrag() error {
 }
 
 func (b *backend) defrag() error {
+	verify.Assert(b.lg != nil, "the logger should not be nil")
 	now := time.Now()
 	isDefragActive.Set(1)
 	defer isDefragActive.Set(0)
@@ -499,7 +506,7 @@ func (b *backend) defrag() error {
 	tmpdb, err := bolt.Open(tdbp, 0o600, &options)
 	if err != nil {
 		temp.Close()
-		if rmErr := os.Remove(temp.Name()); rmErr != nil && b.lg != nil {
+		if rmErr := os.Remove(temp.Name()); rmErr != nil {
 			b.lg.Error(
 				"failed to remove temporary file",
 				zap.String("path", temp.Name()),
@@ -512,16 +519,14 @@ func (b *backend) defrag() error {
 
 	dbp := b.db.Path()
 	size1, sizeInUse1 := b.Size(), b.SizeInUse()
-	if b.lg != nil {
-		b.lg.Info(
-			"defragmenting",
-			zap.String("path", dbp),
-			zap.Int64("current-db-size-bytes", size1),
-			zap.String("current-db-size", humanize.Bytes(uint64(size1))),
-			zap.Int64("current-db-size-in-use-bytes", sizeInUse1),
-			zap.String("current-db-size-in-use", humanize.Bytes(uint64(sizeInUse1))),
-		)
-	}
+	b.lg.Info(
+		"defragmenting",
+		zap.String("path", dbp),
+		zap.Int64("current-db-size-bytes", size1),
+		zap.String("current-db-size", humanize.Bytes(uint64(size1))),
+		zap.Int64("current-db-size-in-use-bytes", sizeInUse1),
+		zap.String("current-db-size-in-use", humanize.Bytes(uint64(sizeInUse1))),
+	)
 
 	defer func() {
 		// NOTE: We should exit as soon as possible because that tx
@@ -584,19 +589,17 @@ func (b *backend) defrag() error {
 	defragSec.Observe(took.Seconds())
 
 	size2, sizeInUse2 := b.Size(), b.SizeInUse()
-	if b.lg != nil {
-		b.lg.Info(
-			"finished defragmenting directory",
-			zap.String("path", dbp),
-			zap.Int64("current-db-size-bytes-diff", size2-size1),
-			zap.Int64("current-db-size-bytes", size2),
-			zap.String("current-db-size", humanize.Bytes(uint64(size2))),
-			zap.Int64("current-db-size-in-use-bytes-diff", sizeInUse2-sizeInUse1),
-			zap.Int64("current-db-size-in-use-bytes", sizeInUse2),
-			zap.String("current-db-size-in-use", humanize.Bytes(uint64(sizeInUse2))),
-			zap.Duration("took", took),
-		)
-	}
+	b.lg.Info(
+		"finished defragmenting directory",
+		zap.String("path", dbp),
+		zap.Int64("current-db-size-bytes-diff", size2-size1),
+		zap.Int64("current-db-size-bytes", size2),
+		zap.String("current-db-size", humanize.Bytes(uint64(size2))),
+		zap.Int64("current-db-size-in-use-bytes-diff", sizeInUse2-sizeInUse1),
+		zap.Int64("current-db-size-in-use-bytes", sizeInUse2),
+		zap.String("current-db-size-in-use", humanize.Bytes(uint64(sizeInUse2))),
+		zap.Duration("took", took),
+	)
 	return nil
 }
 


Test patch: