Skip to content

Commit 185245c

Browse files
authored
fix: prevent index out of range panic in progressLog.Write() (#6001)
* fix: prevent index out of range panic in progressLog.Write() - Move mutex acquisition before all state checks to prevent race conditions - Add safety checks to ensure output buffer always has at least one element - Handle edge case where p.lines is 0 by ensuring minimum 1 element - Add bounds checking before array access operations - Add comprehensive tests for concurrent access and empty slice scenarios Fixes panic: runtime error: index out of range [-1] in progressLog.Write() when concurrent access or edge cases result in empty output buffer. * DRY
1 parent b0f00ca commit 185245c

File tree

2 files changed

+115
-7
lines changed

2 files changed

+115
-7
lines changed

cli/azd/pkg/input/progress_log.go

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -148,9 +148,18 @@ func (p *progressLog) Stop(keepLogs bool) {
148148
// Write implements oi.Writer and updates the internal buffer before flushing it into the screen.
149149
// Calling Write() before Start() or after Stop() is a no-op
150150
func (p *progressLog) Write(logBytes []byte) (int, error) {
151+
// Acquire mutex first to prevent race conditions
152+
p.outputMutex.Lock()
153+
defer p.outputMutex.Unlock()
154+
155+
// Check if component is initialized after acquiring mutex
151156
if p.output == nil {
152157
return len(logBytes), nil
153158
}
159+
160+
// Safety check: ensure output buffer has at least one element
161+
p.ensureOutputBuffer()
162+
154163
maxWidth := p.terminalWidthFn()
155164
if maxWidth <= 0 {
156165
// maxWidth <= 0 means there's no terminal to write and the stdout pipe is mostly connected to a file or a buffer
@@ -159,8 +168,6 @@ func (p *progressLog) Write(logBytes []byte) (int, error) {
159168
}
160169

161170
logsScanner := bufio.NewScanner(strings.NewReader(string(logBytes)))
162-
p.outputMutex.Lock()
163-
defer p.outputMutex.Unlock()
164171

165172
var afterFirstLine bool
166173
for logsScanner.Scan() {
@@ -175,25 +182,34 @@ func (p *progressLog) Write(logBytes []byte) (int, error) {
175182
afterFirstLine = true
176183
}
177184

185+
// Safety check: ensure we have at least one element after slice operations
186+
p.ensureOutputBuffer()
187+
178188
fullLog := log
179-
if p.output[len(p.output)-1] == "" {
189+
lastIndex := len(p.output) - 1
190+
if lastIndex >= 0 && p.output[lastIndex] == "" {
180191
fullLog = p.prefix + log
181192
}
182193
fullLogLen := len(fullLog)
183194

184195
for fullLogLen > 0 {
196+
// Safety check before accessing last element
197+
p.ensureOutputBuffer()
198+
lastIndex = len(p.output) - 1
199+
185200
// Get whatever is the empty space on current line
186-
currentLineRemaining := maxWidth - len(p.output[len(p.output)-1])
201+
currentLineRemaining := maxWidth - len(p.output[lastIndex])
187202
if currentLineRemaining == 0 {
188203
// line is full, use next line. Add prefix first
189204
p.output = append(p.output[1:], p.prefix)
190205
currentLineRemaining = maxWidth - len(p.prefix)
206+
lastIndex = len(p.output) - 1
191207
}
192208

193209
// Choose between writing fullLog (if it is less than currentLineRemaining)
194210
// or writing only currentLineRemaining
195211
writeLen := ix.Min(fullLogLen, currentLineRemaining)
196-
p.output[len(p.output)-1] += fullLog[:writeLen]
212+
p.output[lastIndex] += fullLog[:writeLen]
197213
fullLog = fullLog[writeLen:]
198214
fullLogLen = len(fullLog)
199215
}
@@ -208,8 +224,13 @@ func (p *progressLog) Write(logBytes []byte) (int, error) {
208224
// .Scan() won't add a line break for a line which ends in `\n`
209225
// This is because the next Scan() after \n will find EOF.
210226
// Adding a line break for such case.
211-
if logBytes[len(logBytes)-1] == '\n' {
212-
p.output = append(p.output[1:], p.prefix)
227+
if len(logBytes) > 0 && logBytes[len(logBytes)-1] == '\n' {
228+
// Safety check: ensure we have elements before slice operation
229+
if len(p.output) > 0 {
230+
p.output = append(p.output[1:], p.prefix)
231+
} else {
232+
p.output = []string{p.prefix}
233+
}
213234
}
214235

215236
return len(logBytes), nil
@@ -238,6 +259,19 @@ func (p *progressLog) Header(header string) {
238259

239260
/****************** Not exported method ****************/
240261

262+
// ensureOutputBuffer ensures the output buffer has at least one element.
263+
// This prevents index out of range panics when accessing p.output[len(p.output)-1].
264+
func (p *progressLog) ensureOutputBuffer() {
265+
if len(p.output) == 0 {
266+
// Ensure we have at least 1 line even if p.lines is 0
267+
lineCount := p.lines
268+
if lineCount == 0 {
269+
lineCount = 1
270+
}
271+
p.output = make([]string, lineCount)
272+
}
273+
}
274+
241275
// clearLine override text with empty spaces.
242276
func clearLine() {
243277
tm.Print(tm.ResetLine(""))

cli/azd/pkg/input/progress_log_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,3 +330,77 @@ func Test_progressChangeHeader(t *testing.T) {
330330
snConfig.SnapshotT(t, bufHandler.snap())
331331
pg.Stop(false)
332332
}
333+
334+
func Test_progressLogConcurrentWriteProtection(t *testing.T) {
335+
sizeFn := func() int {
336+
return 40
337+
}
338+
pg := newProgressLogWithWidthFn(5, prefix, title, header, sizeFn)
339+
340+
var bufHandler testBufferHandler
341+
tm.Screen = &bufHandler.Buffer
342+
343+
// Test concurrent access scenario that could cause the panic
344+
pg.Start()
345+
346+
// This should not panic even if called concurrently or after Stop/Start cycles
347+
done := make(chan bool)
348+
go func() {
349+
defer func() {
350+
if r := recover(); r != nil {
351+
t.Errorf("Panic occurred in concurrent write: %v", r)
352+
}
353+
done <- true
354+
}()
355+
356+
for i := 0; i < 100; i++ {
357+
_, err := pg.Write([]byte("concurrent test line\n"))
358+
if err != nil {
359+
t.Errorf("Error in concurrent write: %v", err)
360+
return
361+
}
362+
}
363+
}()
364+
365+
// Try to cause race conditions by stopping/starting
366+
go func() {
367+
for i := 0; i < 10; i++ {
368+
pg.Stop(false)
369+
pg.Start()
370+
}
371+
}()
372+
373+
<-done
374+
pg.Stop(false)
375+
}
376+
377+
func Test_progressLogEmptySliceProtection(t *testing.T) {
378+
sizeFn := func() int {
379+
return 40
380+
}
381+
// Create with 0 lines to test edge case
382+
pg := newProgressLogWithWidthFn(0, prefix, title, header, sizeFn)
383+
384+
var bufHandler testBufferHandler
385+
tm.Screen = &bufHandler.Buffer
386+
387+
// This should not panic even with 0 lines
388+
pg.Start()
389+
_, err := pg.Write([]byte("test line\n"))
390+
require.NoError(t, err)
391+
pg.Stop(false)
392+
393+
// Test with normal lines but force empty slice scenario
394+
pg2 := newProgressLogWithWidthFn(1, prefix, title, header, sizeFn)
395+
pg2.Start()
396+
397+
// Manually create a scenario that could lead to empty slice
398+
pg2.outputMutex.Lock()
399+
pg2.output = []string{} // Force empty slice
400+
pg2.outputMutex.Unlock()
401+
402+
// This should not panic due to safety checks
403+
_, err = pg2.Write([]byte("test after empty\n"))
404+
require.NoError(t, err)
405+
pg2.Stop(false)
406+
}

0 commit comments

Comments
 (0)