hdhomerun stale lock fix and DVR recording deadlock fix#1993
hdhomerun stale lock fix and DVR recording deadlock fix#1993jefflessard wants to merge 5 commits intotvheadend:masterfrom
Conversation
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
Summary of ChangesHello @jefflessard, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two distinct but critical stability issues: a potential deadlock in the DVR recording module and problems with stale tuner locks on HDHomeRun devices. The DVR changes prevent a thread from attempting to join itself during shutdown, which could lead to a deadlock, and ensure proper resource cleanup in error scenarios. The HDHomeRun modifications introduce proactive and reactive measures to identify and clear orphaned tuner locks, which can occur after crashes or improper shutdowns, thereby improving the reliability and availability of tuners. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces two important fixes. First, it resolves a deadlock in the DVR recording logic by preventing a thread from joining itself. The related post-processing logic is also adjusted to handle this case. Second, it adds a mechanism to detect and clear stale locks on HDHomeRun tuners, which can be left behind after a crash, improving the reliability of tuning.
My review focuses on the correctness of these fixes. While the HDHomeRun stale lock handling appears robust, I've found a critical use-after-free vulnerability in the DVR deadlock fix. The mitigation for a memory leak inadvertently creates a dangling pointer that is returned by the DVR thread. I've provided a detailed comment and a code suggestion to address this.
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses two critical issues in tvheadend: stale HDHomeRun device locks that prevent tuning, and a potential deadlock when stopping DVR recordings from within the worker thread.
Key Changes:
- Adds automatic detection and clearing of orphaned HDHomeRun tuner locks from previous sessions
- Implements deadlock prevention in DVR recording cleanup by detecting self-join scenarios
- Handles postprocessing memory management in error paths
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/input/mpegts/tvhdhomerun/tvhdhomerun_private.h | Adds declaration for the new stale lock clearing function |
| src/input/mpegts/tvhdhomerun/tvhdhomerun_frontend.c | Implements stale lock detection/clearing and integrates it into the tuning workflow |
| src/input/mpegts/tvhdhomerun/tvhdhomerun.c | Clears orphaned locks on all tuners during device discovery |
| src/dvr/dvr_rec.c | Implements deadlock prevention and manages postproc memory in error paths |
|
|
||
| for (j = 0; j < dInfo->tuner_count; j++) { | ||
| hd = hdhomerun_device_create(dInfo->device_id, dInfo->ip_addr, j, NULL); | ||
|
|
There was a problem hiding this comment.
Trailing whitespace should be removed for code consistency.
src/dvr/dvr_rec.c
Outdated
|
|
||
| dvr_stop_recording(de, code == SM_CODE_NO_SPACE ? SM_CODE_NO_SPACE : SM_CODE_INVALID_TARGET, 1, 0); | ||
| } | ||
|
|
There was a problem hiding this comment.
Trailing whitespace should be removed for code consistency.
There was a problem hiding this comment.
Its not talking about the empty line but about the whitespaces in the empty line, everywhere else in this file empty lines do not have any whitespaces.
|
I am sorry if my responses may sound harsh but this copilot automated review is very deceiving, of the level of a rookie developer. It basically focus on minor details (maybe except the dangling pointer) rather than understanding the importance of the fix and why it has been done this way. |
|
/gemini It looks like your detailed comment was not added, could you please add it again? |
|
Hello @Flole998, Thank you for reaching out. My initial comment on this pull request was a high-level summary of the changes. Detailed review comments are typically posted directly on the lines of code that are being discussed. I understand from @jefflessard's previous comment that the initial review might have focused too much on minor details. I appreciate this feedback and will strive to provide more contextual and impactful suggestions in future reviews. If you would like me to perform a full, detailed code review of the current state of the pull request, please use the command Please let me know if you have any other questions or if there's anything else I can assist with. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two important fixes for HDHomeRun and DVR functionality. The HDHomeRun changes correctly handle stale tuner locks by clearing them on device discovery and before attempting to acquire a lock. This should improve reliability, especially after a Tvheadend crash. The DVR changes address a deadlock that could occur when a recording thread tries to join itself. The fix is sound, but I've identified a critical use-after-free vulnerability that is introduced as a side effect, which needs to be addressed.
src/dvr/dvr_rec.c
Outdated
| if (prch->prch_muxer) | ||
| dvr_thread_epilog(de, postproc); | ||
|
|
||
| free(postproc); |
There was a problem hiding this comment.
This free(postproc) call correctly prevents a memory leak that would otherwise occur due to the deadlock prevention logic. However, it introduces a use-after-free vulnerability.
The dvr_thread function, which calls dvr_thread_rec_start, is unaware that postproc has been freed. When dvr_thread exits, it returns this dangling pointer. If another thread calls pthread_join on dvr_thread, it will receive a pointer to freed memory, which could lead to a crash or other undefined behavior.
A robust solution is to ensure the caller is aware of the memory being freed. One way to do this is to change the function signature to accept a pointer to the pointer:
dvr_thread_rec_start(..., char **postproc)
Then, you can nullify the pointer in the caller's scope after freeing it:
free(*postproc);
*postproc = NULL;This would require updating the call site in dvr_thread to pass &postproc, and dereferencing postproc within this function. Although the call site is outside the current diff, this change is crucial for memory safety.
There was a problem hiding this comment.
Yep! That's what I was saying in previous comments: passing postproc pointer by reference and cha 17A6 nging the signature (removing const) is a non-issue.
|
|
@Flole998 latests 2 commits should now properly address the discussed comments. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two important fixes: one for a deadlock in DVR recording and another for handling stale locks on HDHomeRun tuners. The deadlock fix in dvr_rec.c correctly prevents a thread from joining itself, and the related changes to handle resource cleanup in this scenario are well-implemented. The stale lock fix for HDHomeRun devices proactively clears orphaned locks at device discovery and before tuning, which should improve reliability.
I've found two issues of high severity:
- A potential resource leak in the DVR deadlock fix, where a muxer might not be cleaned up.
- A memory leak in the new HDHomeRun stale lock handling logic.
Details and suggestions are in the line comments. Apart from these issues, the changes are good.
| if (postproc && prch->prch_muxer) | ||
| dvr_thread_epilog(de, postproc); |
There was a problem hiding this comment.
The condition for calling dvr_thread_epilog has been changed from if (prch->prch_muxer) to if (postproc && prch->prch_muxer). This is incorrect and will lead to a resource leak. If no post-processing script is configured, postproc will be NULL, and dvr_thread_epilog will not be called. This means the muxer (prch->prch_muxer) will not be closed or destroyed. The dvr_thread_epilog function handles a NULL postproc argument safely, so the original condition was correct. Please revert this condition to if (prch->prch_muxer).
if (prch->prch_muxer)
dvr_thread_epilog(de, postproc);| bool tvhdhomerun_clear_stale_lock(struct hdhomerun_device_t *hd) | ||
| { | ||
| char *lockkey_owner = NULL; | ||
| char local_ip_str[64]; | ||
| uint32_t local_ip = 0; | ||
| int res; | ||
|
|
||
| assert(hd != NULL); | ||
|
|
||
| // Get the local IP address as seen by the device | ||
| local_ip = hdhomerun_device_get_local_machine_addr(hd); | ||
| if (local_ip == 0) { | ||
| tvhwarn(LS_TVHDHOMERUN, "failed to query local_ip"); | ||
| return false; | ||
| } | ||
|
|
||
| // Convert IP to dotted-quad string format | ||
| snprintf(local_ip_str, sizeof(local_ip_str), "%u.%u.%u.%u", | ||
| (local_ip >> 24) & 0xFF, | ||
| (local_ip >> 16) & 0xFF, | ||
| (local_ip >> 8) & 0xFF, | ||
| local_ip & 0xFF); | ||
|
|
||
| tvhtrace(LS_TVHDHOMERUN, "local_ip=%s", local_ip_str); | ||
|
|
||
| // Query current lock holder | ||
| res = hdhomerun_device_get_tuner_lockkey_owner(hd, &lockkey_owner); | ||
| if (res < 0) { | ||
| tvhwarn(LS_TVHDHOMERUN, "failed to query lockkey (res=%d)", res); | ||
| return false; | ||
| } | ||
|
|
||
| if (lockkey_owner == NULL || strcmp(lockkey_owner, "none") == 0) { | ||
| tvhtrace(LS_TVHDHOMERUN, "no lockkey set"); | ||
| return false; | ||
| } | ||
|
|
||
| // Check if the lock is held by ourselves | ||
| if (strcmp(lockkey_owner, local_ip_str) != 0) { | ||
| tvhinfo(LS_TVHDHOMERUN, | ||
| "tuner locked by different host: %s (not clearing)", | ||
| lockkey_owner); | ||
| return false; | ||
| } | ||
|
|
||
| // Lock is held by OUR IP - this is an orphaned lock from a previous session | ||
| tvhinfo(LS_TVHDHOMERUN, | ||
| "detected orphaned self-lock (%s), force-releasing", | ||
| lockkey_owner); | ||
|
|
||
| // Force-release the orphaned lock | ||
| hdhomerun_device_tuner_lockkey_force(hd); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The lockkey_owner string, which is allocated by hdhomerun_device_get_tuner_lockkey_owner, is not freed in any of the return paths of this function. This will cause a memory leak. You should free(lockkey_owner) before each return statement after hdhomerun_device_get_tuner_lockkey_owner has been called. A goto cleanup pattern might be cleaner to ensure it's always freed.
There was a problem hiding this comment.
Thanks for the check, but this is a false positive. hdhomerun_device_get_tuner_lockkey_owner returns the pointer produced by hdhomerun_control_get, and the control API explicitly documents that the returned string is owned by the control-socket and "will remain valid until the next call to a control sock function." Callers must not free that pointer.
- hdhomerun_control.h documents the ownership/lifetime: the char* returned via the pvalue/powner parameter is an internal buffer owned by the control socket and "will remain valid until the next call to a control sock function." It is not caller-owned memory to free.
- hdhomerun_device_get_tuner_lockkey_owner simply forwards to the control API:
- Implementation: it builds "/tuner%u/lockkey" then returns hdhomerun_control_get(hd->cs, name, powner, NULL);
- So the pointer you get from hdhomerun_device_get_tuner_lockkey_owner is exactly the pointer returned by hdhomerun_control_get and therefore has the same ownership rules.
Consequences:
- Calling free(lockkey_owner) in tvhdhomerun_clear_stale_lock would be wrong (double-free / free of library-owned buffer) and can crash or corrupt memory.
- The current code that does not free lockkey_owner is correct. If the code needs the string past the next control-socket call (or past the lifetime of the control socket), it should strdup() it and free its own copy later.
|
Hello, I’ve reviewed this PR regarding the lockkey issue #2022 Regarding the forced unlock solution, I believe this behavior should be configurable and disabled by default, ideally via the tuner settings. There are scenarios where tvheadend runs on the same machine or IP as other software, and in such cases forcibly releasing the stream could cause conflicts or unintended disruptions. For this reason, I think this should be an optional feature that users can explicitly enable when needed. PS: I performed several tests and observed that the HDHomeRun firmware does not release the tuner after 30 seconds without data, including cases where tvheadend is restarted or the consuming machine is fully rebooted. |
No description provided.