Skip to content

Commit 14f3b4c

Browse files
GMHDBJDGMHDBJD
authored
precheck log file permission
precheck log file permission
2 parents 85a82d0 + 3b10a50 commit 14f3b4c

2 files changed

Lines changed: 101 additions & 19 deletions

File tree

‎log.go‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"errors"
1919
"fmt"
2020
"os"
21+
"path/filepath"
2122
"sync"
2223
"sync/atomic"
2324
"time"
@@ -174,12 +175,42 @@ func (s *lockWithTimeoutWrapper) Sync() error {
174175

175176
// initFileLog initializes file based logging options.
176177
func initFileLog(cfg *FileLogConfig) (*lumberjack.Logger, error) {
178+
// Verify file permissions early to prevent silent failures.
179+
// Since lumberjack creates log files lazily, permission issues wouldn't be detected
180+
// until the first write attempt. This is problematic when tools redirect their
181+
// error output to these log files.
182+
// e.g. dumpling use the log file as ErrorOutputPath, but the log file will not be created
183+
// if there has permission issues, and dumpling will not print any error message.
184+
185+
// Create the directory if it doesn't exist
186+
dir := filepath.Dir(cfg.Filename)
187+
if err := os.MkdirAll(dir, 0755); err != nil {
188+
return nil, fmt.Errorf("cannot create log directory: %w", err)
189+
}
190+
191+
// Check if the path is a directory which is invalid
177192
if st, err := os.Stat(cfg.Filename); err == nil {
178193
if st.IsDir() {
179194
return nil, errors.New("can't use directory as log file name")
180195
}
181-
} else if !os.IsNotExist(err) {
182-
return nil, err
196+
197+
// Check if the file is writable
198+
file, err := os.OpenFile(cfg.Filename, os.O_WRONLY|os.O_APPEND, 0666)
199+
if err != nil {
200+
return nil, fmt.Errorf("can't write to log file: %w", err)
201+
}
202+
file.Close()
203+
} else if os.IsNotExist(err) {
204+
// File doesn't exist, verify we can create it
205+
file, err := os.Create(cfg.Filename)
206+
if err != nil {
207+
return nil, fmt.Errorf("can't create log file: %w", err)
208+
}
209+
file.Close()
210+
// Remove the empty file since lumberjack will create it
211+
os.Remove(cfg.Filename)
212+
} else {
213+
return nil, fmt.Errorf("error checking log file: %w", err)
183214
}
184215

185216
if cfg.MaxSize == 0 {

‎zap_log_test.go‎

Lines changed: 68 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"math"
2020
"net"
2121
"os"
22+
"path/filepath"
2223
"strings"
2324
"testing"
2425
"time"
@@ -99,16 +100,16 @@ func TestLog(t *testing.T) {
99100
zap.Duration("duration", 10*time.Second),
100101
)
101102
ts.assertMessages(
102-
`[INFO] [zap_log_test.go:49] ["failed to fetch URL"] [url=http://example.com] [attempt=3] [backoff=1s]`,
103-
`[INFO] [zap_log_test.go:54] ["failed to \"fetch\" [URL]: http://example.com"]`,
104-
`[DEBUG] [zap_log_test.go:55] ["Slow query"] [sql="SELECT * FROM TABLE\n\tWHERE ID=\"abc\""] [duration=1.3s] ["process keys"=1500]`,
105-
`[INFO] [zap_log_test.go:61] [Welcome]`,
106-
`[INFO] [zap_log_test.go:62] ["Welcome TiDB"]`,
107-
`[INFO] [zap_log_test.go:63] [欢迎]`,
108-
`[INFO] [zap_log_test.go:64] ["欢迎来到 TiDB"]`,
109-
`[WARN] [zap_log_test.go:65] [Type] [Counter=NaN] [Score=+Inf]`,
110-
`[INFO] [zap_log_test.go:70] ["new connection"] [connID=1] [traceID=dse1121]`,
111-
`[INFO] [zap_log_test.go:71] ["Testing typs"] [filed1=noquote] `+
103+
`[INFO] [zap_log_test.go:50] ["failed to fetch URL"] [url=http://example.com] [attempt=3] [backoff=1s]`,
104+
`[INFO] [zap_log_test.go:55] ["failed to \"fetch\" [URL]: http://example.com"]`,
105+
`[DEBUG] [zap_log_test.go:56] ["Slow query"] [sql="SELECT * FROM TABLE\n\tWHERE ID=\"abc\""] [duration=1.3s] ["process keys"=1500]`,
106+
`[INFO] [zap_log_test.go:62] [Welcome]`,
107+
`[INFO] [zap_log_test.go:63] ["Welcome TiDB"]`,
108+
`[INFO] [zap_log_test.go:64] [欢迎]`,
109+
`[INFO] [zap_log_test.go:65] ["欢迎来到 TiDB"]`,
110+
`[WARN] [zap_log_test.go:66] [Type] [Counter=NaN] [Score=+Inf]`,
111+
`[INFO] [zap_log_test.go:71] ["new connection"] [connID=1] [traceID=dse1121]`,
112+
`[INFO] [zap_log_test.go:72] ["Testing typs"] [filed1=noquote] `+
112113
`[filed2="in quote"] [urls="[http://mock1.com:2347,http://mock2.com:2432]"] `+
113114
`[urls-peer="[t1,\"t2 fine\"]"] ["store ids"="[1,4,5]"] [object="{username=user1}"] `+
114115
`[object2="{username=\"user 2\"}"] [binary="YWIxMjM="] ["is processed"=true] `+
@@ -231,8 +232,8 @@ func TestLogJSON(t *testing.T) {
231232
"backoff", time.Second,
232233
)
233234
logger.With(zap.String("connID", "1"), zap.String("traceID", "dse1121")).Info("new connection")
234-
ts.assertMessages("{\"level\":\"INFO\",\"caller\":\"zap_log_test.go:228\",\"message\":\"failed to fetch URL\",\"url\":\"http://example.com\",\"attempt\":3,\"backoff\":\"1s\"}",
235-
"{\"level\":\"INFO\",\"caller\":\"zap_log_test.go:233\",\"message\":\"new connection\",\"connID\":\"1\",\"traceID\":\"dse1121\"}")
235+
ts.assertMessages("{\"level\":\"INFO\",\"caller\":\"zap_log_test.go:229\",\"message\":\"failed to fetch URL\",\"url\":\"http://example.com\",\"attempt\":3,\"backoff\":\"1s\"}",
236+
"{\"level\":\"INFO\",\"caller\":\"zap_log_test.go:234\",\"message\":\"new connection\",\"connID\":\"1\",\"traceID\":\"dse1121\"}")
236237
}
237238

238239
func TestRotateLogWithCompress(t *testing.T) {
@@ -288,23 +289,73 @@ func TestCompressError(t *testing.T) {
288289
}
289290

290291
func TestLogFileNoPermission(t *testing.T) {
291-
tempDir, _ := os.MkdirTemp("/tmp", "tests-log")
292-
defer os.RemoveAll(tempDir)
292+
tempDir := t.TempDir()
293+
294+
// Directory permission denied
295+
dirPath := filepath.Join(tempDir, "noperm-dir")
296+
require.NoError(t, os.Mkdir(dirPath, 0755))
293297
conf := &Config{
294298
Level: "info",
295299
File: FileLogConfig{
296-
Filename: tempDir + "/test.log",
300+
Filename: filepath.Join(dirPath, "test.log"),
297301
MaxSize: 1,
298302
},
299303
}
300304
_, _, err := InitLogger(conf)
301305
require.NoError(t, err)
306+
require.NoError(t, os.Chmod(dirPath, 0))
307+
_, _, err = InitLogger(conf)
308+
require.Contains(t, err.Error(), "permission denied")
309+
require.NoError(t, os.Chmod(dirPath, 0755))
310+
311+
// Directory exists but doesn't allow file creation
312+
readOnlyDirPath := filepath.Join(tempDir, "readonly-dir")
313+
require.NoError(t, os.Mkdir(readOnlyDirPath, 0755))
314+
require.NoError(t, os.Chmod(readOnlyDirPath, 0555)) // Read-only directory
315+
conf = &Config{
316+
Level: "info",
317+
File: FileLogConfig{
318+
Filename: filepath.Join(readOnlyDirPath, "test.log"),
319+
MaxSize: 1,
320+
},
321+
}
322+
_, _, err = InitLogger(conf)
323+
require.Contains(t, err.Error(), "permission denied")
324+
require.NoError(t, os.Chmod(readOnlyDirPath, 0755))
325+
326+
// Using a directory as log file
327+
dirLogPath := filepath.Join(tempDir, "dir-as-log")
328+
require.NoError(t, os.Mkdir(dirLogPath, 0755))
329+
conf = &Config{
330+
Level: "info",
331+
File: FileLogConfig{
332+
Filename: dirLogPath,
333+
MaxSize: 1,
334+
},
335+
}
336+
_, _, err = InitLogger(conf)
337+
require.Contains(t, err.Error(), "can't use directory as log file name")
302338

303-
err = os.Chmod(tempDir, 0)
339+
// File exists but is not writable
340+
filePath := filepath.Join(tempDir, "readonly.log")
341+
file, err := os.Create(filePath)
304342
require.NoError(t, err)
343+
file.Close()
344+
require.NoError(t, os.Chmod(filePath, 0444))
305345

346+
// Ensure parent directory is created successfully
347+
nestedPath := filepath.Join(tempDir, "nested/path/to")
348+
conf = &Config{
349+
Level: "info",
350+
File: FileLogConfig{
351+
Filename: filepath.Join(nestedPath, "test.log"),
352+
MaxSize: 1,
353+
},
354+
}
306355
_, _, err = InitLogger(conf)
307-
require.Contains(t, err.Error(), "permission denied")
356+
require.NoError(t, err)
357+
_, err = os.Stat(nestedPath)
358+
require.NoError(t, err)
308359
}
309360

310361
// testLogSpy is a testing.TB that captures logged messages.

0 commit comments

Comments
 (0)