Conversation
There was a problem hiding this comment.
libwallet Security & Concurrency Review
Summary:
- 1 CRITICAL issue (potential deadlock)
- 1 WARNING (lock held during I/O)
- 1 SUGGESTION (initialization order)
Security Notes:
✅ No secrets logged
✅ Error messages appropriately generic
✅ Mutex patterns mostly correct
Details:
See inline comments for specific issues.
| } | ||
| wallets = make(map[string]*wallet) | ||
|
|
||
| // Stop all remaining background processes and wait for them to stop. |
There was a problem hiding this comment.
[CRITICAL] Potential deadlock: Context cancellation with mutex held.
Current:
walletsMtx.Lock()
defer walletsMtx.Unlock()
// ...
cancelMainCtx()
wg.Wait()Issue: If any goroutine in wg tries to acquire walletsMtx (e.g., in wallet operations), it will deadlock because wg.Wait() is called while holding the mutex.
Suggested:
walletsMtx.Lock()
// ... close wallets ...
walletsMtx.Unlock()
// Cancel and wait AFTER releasing the mutex
cancelMainCtx()
wg.Wait()
logMtx.Lock()
// ... close logger ...Why: Background goroutines must be able to acquire locks during shutdown. Holding locks during wg.Wait() is a common deadlock pattern.
| log = logBackend.SubLogger("APP") | ||
| logMtx.Unlock() | ||
|
|
||
| mainCtx, cancelMainCtx = context.WithCancel(context.Background()) |
There was a problem hiding this comment.
[SUGGESTION] Initialization order could be improved.
Current: Context created after setting initialized = true flag (line 77).
Suggested: Move mainCtx, cancelMainCtx = context.WithCancel(context.Background()) to before setting initialized = true (around line 75).
Why: If initialization is checked between line 77 and 76, the context might not be ready. Minor race condition.
| if !initialized { | ||
| return "", errors.New("not initialized") | ||
| } | ||
|
|
There was a problem hiding this comment.
[WARNING] RLock held during potentially slow operation.
Current: logMtx.RLock() held during log.Debug() which may involve I/O.
Consider: If logging is fast and non-blocking in this implementation, this may be acceptable. However, best practice is to minimize lock duration.
Suggested pattern (if needed):
logMtx.RLock()
logger := log
logMtx.RUnlock()
logger.Debug("libwallet mobile shutting down")Why: Avoid holding locks during I/O operations to prevent blocking other goroutines.
test 2