Mohamed Ayman commented: https://gitlab.rtems.org/rtems/rtos/rtems/-/work_items/2129#note_158723 Hi @sebhub ,@gedare , @chris , @opticron , @joel I've been digging into #2129 ("sync() implementation is dangerous") and wanted to share what I found, since it looks like the ticket may have been overtaken by events without anyone going back to close it out. For context on how I ended up here: the ticket describes three weaknesses in `sync()` no protection against concurrent `sync()` calls, no Newlib locks meaning other tasks shouldn't touch file operations while `sync()` runs, and undefined behavior if a task is deleted mid-`sync()`. That's still exactly what the ticket says today, filed against 4.11/HEAD, milestone later moved to 5.0, and it's sat open ever since. What I found going through the git history of `cpukit/libcsupport/src/sync.c`: back when this ticket was filed, `sync()` worked by walking each task's Newlib reentrancy structure and every `FILE*` stream open on it, calling `fsync()`/`fdatasync()` on each one it found. That walk is exactly the mechanism the three weaknesses are about — nothing stopped a task from being deleted, or a file from being closed, while `sync()` was still walking it. That implementation doesn't exist anymore. In commit `3ccfb58311` ("Synchronize all file descriptors in sync()", May 13 2022, your commit), `sync()` was rewritten from scratch to just loop over the fd table directly and call `fsync()`/`fdatasync()` on each fd — no more per-task walk at all. The commit message says it closes #4656, which as far as I can tell was about `sync()` missing files that weren't associated with a `FILE*` stream, not about the thread-safety issue here. So the fix for a completely different bug happened to remove the exact thing this ticket was worried about, as a side effect. I looked for a merge request tied to that commit and couldn't find one not sure if it went in as a direct commit or if I just couldn't locate the MR. I didn't want to just take that at face value, so I wrote a stress test to check it myself: two tasks hammering `sync()` back to back, one task doing continuous `open()`/`write()`/`close()`, and one task continuously creating and exiting other tasks — all running in parallel on 4 real cores (`sis -leon3 -m 4`), against a real FAT filesystem mounted on a RAM disk (IMFS's `fsync_h` turned out to be a no-op, so I didn't want to test against that). 30 runs, 20,000 iterations per task, no crashes, hangs, or failures in any of them. So from where I'm sitting: the specific failure mode this ticket describes doesn't seem to exist in current `main` anymore, but I want to be upfront about what I haven't checked I only tested FAT (not RFS/littlefs/JFFS2), and I only tested clean task exit, not a forced `rtems_task_delete()` mid-I/O, which seems like it could still be a real risk given there's no explicit lock serializing `sync()` even now. Given you filed this originally and wrote the 2022 rewrite is this still something you consider open, or would you be comfortable closing it referencing that commit? And if there's a specific scenario you still had in mind that I'm not covering, I'm happy to extend the test and dig further.. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/work_items/2129#note_158723 You're receiving this email because of your account on gitlab.rtems.org. Unsubscribe from this thread: https://gitlab.rtems.org/-/sent_notifications/5-2gxer6bjhru83hgmi2dq2se5o-1d/unsubscribe | Manage all notifications: https://gitlab.rtems.org/-/profile/notifications | Help: https://gitlab.rtems.org/help
_______________________________________________ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
