Skip to content

drivers/serial: hold xmit.lock around echo in uart_readv()#19391

Open
94xhn wants to merge 1 commit into
apache:masterfrom
94xhn:fix-uart-echo-xmit-lock
Open

drivers/serial: hold xmit.lock around echo in uart_readv()#19391
94xhn wants to merge 1 commit into
apache:masterfrom
94xhn:fix-uart-echo-xmit-lock

Conversation

@94xhn

@94xhn 94xhn commented Jul 9, 2026

Copy link
Copy Markdown

Summary

Fixes #14845. uart_putxmitchar() manipulates dev->xmit.head/dev->xmit.buffer directly with no locking of its own — every other caller takes dev->xmit.lock first (uart_write() locks once around its whole per-character write loop). uart_readv()'s ECHO handling calls uart_putxmitchar() in two places (the backspace/delete erase sequence, and normal character echo) without ever taking that lock, so a concurrent uart_write() and a local echo triggered by uart_readv() can race on the same circular transmit buffer state, corrupting it.

Fix

Take dev->xmit.lock around each echo's uart_putxmitchar() call(s), matching what uart_write() already does elsewhere in this same file.

uart_readv() holds dev->recv.lock for the duration of its own read loop, so this introduces recv.lockxmit.lock nesting. I checked every other use of recv.lock/xmit.lock in this file and no other code path ever acquires recv.lock while already holding xmit.lock (nothing outside uart_readv() touches recv.lock at all), so this doesn't create a new lock-ordering cycle/deadlock risk.

Testing

I don't have real serial hardware or a working local NuttX sim build environment (WSL crashed on this machine and I wasn't able to get it running again), so I can't provide a build/run log or a genuine concurrent-echo-vs-write repro. What I did instead: read every call site of uart_putxmitchar() and every use of xmit.lock/recv.lock in drivers/serial/serial.c to confirm (a) uart_putxmitchar() really has no internal synchronization, (b) uart_write()'s existing lock/unlock pattern is the one to mirror, and (c) the lock-ordering argument above. Happy to add a stronger repro or run sim:nsh if a maintainer can point me to (or confirm) a way to trigger it, e.g. via a two-thread test that concurrently write()s and triggers local echo on the same tty.

Generative AI

I used generative AI tools when creating this PR, but a human has checked the code and is responsible for the code and the description above.

uart_putxmitchar() manipulates dev->xmit.head/buffer directly with
no internal locking - every other caller (uart_write()) takes
dev->xmit.lock first. uart_readv()'s ECHO handling (both the
backspace/delete erase sequence and the normal character echo)
calls uart_putxmitchar() without taking that lock, so a concurrent
uart_write() and a local echo can race on the same circular buffer
state, corrupting it.

Take dev->xmit.lock around each echo's uart_putxmitchar() calls,
matching what uart_write() already does. uart_readv() holds
dev->recv.lock for its own duration, but no other code path ever
acquires recv.lock while holding xmit.lock, so nesting xmit.lock
inside the existing recv.lock scope here doesn't introduce a new
lock-ordering cycle.

Fixes apache#14845

Signed-off-by: yi chen <94xhn1@gmail.com>
@github-actions github-actions Bot added Area: Drivers Drivers issues Size: XS The size of the change in this PR is very small labels Jul 10, 2026
@github-actions

Copy link
Copy Markdown

MemBrowse Memory Report

arduino-mega2560

  • flash: .text +18 B (+0.0%, 67,218 B / 262,144 B, total: 26% used)

esp32-devkitc

  • ROM: .flash.text +56 B (+0.0%, 122,708 B / 4,194,272 B, total: 3% used)
  • irom0_0_seg: .flash.text +56 B (+0.1%, 87,076 B / 3,342,304 B, total: 3% used)

qemu-armv8a

  • Code: .text.uart_readv +48 B (+0.0%, 315,358 B)

qemu-intel64

  • Code: .text +48 B (+0.0%, 8,655,397 B)

rx65n-rsk2mb

  • ROM: .text +32 B (+0.0%, 85,640 B / 2,097,152 B, total: 4% used)

s698pm-dkit

  • Code: .text +80 B (+0.0%, 357,360 B)

stm32-nucleo-f103rb

  • flash: .text +36 B (+0.1%, 33,772 B / 131,072 B, total: 26% used)
    No memory changes detected for:
  • hifive1-revb
  • mirtoo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Drivers Drivers issues Size: XS The size of the change in this PR is very small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] possible locking issue in uart_putxmitchar

2 participants