Implement Message Queue Limit to Prevent Memory Exhaustion in Embedded Devices #629
Conversation
WalkthroughThe primary changes introduce enhanced network event handling and control flow for various platforms. Key additions include managing sockets' dynamic allocation, optimizing wakeup signaling, and incorporating queue length checks to prevent memory exhaustion in embedded environments. Changes
Sequence Diagram(s)sequenceDiagram
participant NetworkThread
participant EventQueue
participant EventThread
Note over NetworkThread,EventQueue: Initialization and Queue Limit Check
NetworkThread->>EventQueue: Check queue length
EventQueue-->>NetworkThread: Response (length)
alt Queue not full
NetworkThread->>EventQueue: Enqueue event
EventQueue-->>EventThread: Wakeup signal
end
EventThread->>NetworkThread: Process event from queue
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed. ℹ️ To verify your latest change (e6ac425), label this PR with |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- api/oc_network_events.c (3 hunks)
- api/oc_network_events_internal.h (1 hunks)
- port/linux/ipadapter.c (10 hunks)
- port/linux/oc_config.h (2 hunks)
- port/linux/tcpadapter.c (1 hunks)
- port/linux/tcpadapter.h (1 hunks)
- port/oc_connectivity_internal.h (1 hunks)
Files skipped from review due to trivial changes (1)
- port/linux/oc_config.h
Additional comments not posted (12)
port/linux/tcpadapter.h (2)
50-50: The functiontcp_add_socks_to_rfd_setis well-documented and clear in its purpose.
56-61: The addition oftcp_add_controlflow_socks_to_rfd_setenhances control over TCP sockets by managing control flow sockets separately.api/oc_network_events_internal.h (1)
81-87: The functionoc_get_network_events_queue_lengthprovides essential functionality for monitoring the length of the network event queue, which is critical for resource management in constrained environments.port/oc_connectivity_internal.h (1)
67-72: The functionoc_connectivity_wakeupunder theOC_DYNAMIC_ALLOCATIONflag is a critical addition for managing network event consumption dynamically.api/oc_network_events.c (2)
60-62: Proper handling of network interface states with flags reset after processing ensures robust network monitoring.
68-73: The dynamic allocation checks and the conditional wake-up signal are crucial for managing network event queue limits effectively.port/linux/tcpadapter.c (1)
273-276: The functiontcp_add_controlflow_socks_to_rfd_setis a valuable addition for efficiently managing control flow in TCP connectivity.port/linux/ipadapter.c (5)
681-693: Ensure proper handling of shutdown signals.The function
process_wakeup_signalcorrectly checks for a shutdown signal and clears the file descriptor set appropriately. This is crucial for graceful shutdown handling.
1003-1011: Correctly manage control flow file descriptors.The function
add_control_flow_rfdsproperly adds the necessary file descriptors for control flow management to the set. This is essential for ensuring that control flow events are handled even when the network event queue is full.
1045-1054: Handle full network event queues appropriately.The logic to handle situations when the network event queue is full is correctly implemented. By resetting the file descriptor set and only adding control flow descriptors, the system ensures that it remains responsive.
1575-1586: Ensure robust signaling to the network event thread.The function
signal_event_threaduses a loop to handleEINTRerrors during thewriteoperation, which is a good practice to ensure that the signal is sent reliably.
1611-1627: Properly wake up all network event threads.The function
oc_connectivity_wakeupcorrectly iterates over all devices and signals each network event thread. This is crucial for ensuring that all threads respond to wake-up calls.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- api/oc_network_events.c (3 hunks)
- api/oc_network_events_internal.h (1 hunks)
- port/linux/ipadapter.c (10 hunks)
- port/linux/tcpadapter.c (1 hunks)
- port/linux/tcpadapter.h (1 hunks)
- port/oc_connectivity_internal.h (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- api/oc_network_events.c
- api/oc_network_events_internal.h
- port/linux/tcpadapter.c
- port/linux/tcpadapter.h
- port/oc_connectivity_internal.h
Additional comments not posted (6)
port/linux/ipadapter.c (6)
39-39: Include of"port/oc_random.h"is added to support random operations in the file.
681-693: The functionprocess_wakeup_signalhas been renamed and modified to handle a wakeup signal instead of a shutdown. This change aligns with the PR's objective to enhance control flow management.
1008-1018: The functionadd_control_flow_rfdshas been added to manage control flow file descriptors. This function is part of the new dynamic allocation strategy to handle network events more efficiently.
1052-1062: The logic to handle full network event queues has been implemented here. When the queue is full, only control flow descriptors are added, which is a critical part of preventing memory exhaustion under high load or DDOS conditions.
1583-1595: The functionsignal_event_threadhas been introduced to signal the event thread, which is part of the enhancements to manage network events more effectively.
1621-1636: The functionoc_connectivity_wakeuphas been added to wake up the connectivity on all devices. This function is crucial for managing network events dynamically across different devices.
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
port/android/ipadapter.c (1)
826-826: Handle potential read errors more explicitly.Consider adding explicit error handling or logging for the
readoperation on the wakeup pipe to aid in debugging and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- api/oc_network_events.c (5 hunks)
- api/oc_network_events_internal.h (2 hunks)
- port/android/ipadapter.c (6 hunks)
- port/android/ipcontext.h (1 hunks)
- port/android/oc_config.h (1 hunks)
- port/esp32/adapter/include/oc_config.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (6 hunks)
- port/esp32/adapter/src/ipcontext.h (1 hunks)
- port/linux/ipadapter.c (11 hunks)
- port/linux/ipcontext.h (1 hunks)
- port/linux/oc_config.h (2 hunks)
- port/oc_connectivity_internal.h (1 hunks)
Files skipped from review due to trivial changes (2)
- port/android/ipcontext.h
- port/esp32/adapter/src/ipcontext.h
Files skipped from review as they are similar to previous changes (5)
- api/oc_network_events.c
- api/oc_network_events_internal.h
- port/linux/ipadapter.c
- port/linux/oc_config.h
- port/oc_connectivity_internal.h
Additional comments not posted (7)
port/linux/ipcontext.h (1)
73-73: Renamingshutdown_pipetowakeup_pipealigns with the enhanced signaling mechanism objectives.Verification successful
The renaming of
shutdown_pipetowakeup_pipehas been consistently applied across the codebase, including Linux, Android, and ESP32 platforms.
port/linux/ipcontext.hport/linux/ipadapter.cport/android/ipcontext.hport/android/ipadapter.cport/esp32/adapter/src/ipcontext.hport/esp32/adapter/src/ipadapter.cScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `wakeup_pipe` across the platform. # Test: Search for the usage of `wakeup_pipe` and `shutdown_pipe`. Expect: Only occurrences of `wakeup_pipe`. rg --type c $'wakeup_pipe|shutdown_pipe'Length of output: 1955
port/esp32/adapter/include/oc_config.h (1)
65-68: The addition ofOC_DEVICE_MAX_NUM_CONCURRENT_REQUESTSis crucial for managing memory resources effectively on devices.Verification successful
The macro
OC_DEVICE_MAX_NUM_CONCURRENT_REQUESTSis consistently defined and used across multiple files. Here are the locations where it is defined and used:
Defined in:
port/esp32/adapter/include/oc_config.hport/android/oc_config.hport/linux/oc_config.hUsed in:
port/linux/ipadapter.capi/oc_network_events.cThe integration appears consistent and correct across the platform.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `OC_DEVICE_MAX_NUM_CONCURRENT_REQUESTS` across the platform. # Test: Search for the usage of `OC_DEVICE_MAX_NUM_CONCURRENT_REQUESTS`. Expect: Consistent usage across configuration files. rg --type c $'OC_DEVICE_MAX_NUM_CONCURRENT_REQUESTS'Length of output: 1166
port/android/oc_config.h (1)
65-68: The addition ofOC_DEVICE_MAX_NUM_CONCURRENT_REQUESTSis crucial for managing memory resources effectively on devices.Verification successful
The macro
OC_DEVICE_MAX_NUM_CONCURRENT_REQUESTSis consistently defined and used across different platform configurations and source files.
port/esp32/adapter/include/oc_config.hport/linux/oc_config.hport/linux/ipadapter.cport/android/oc_config.hapi/oc_network_events.cThis confirms that the macro is well-integrated across the platform.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `OC_DEVICE_MAX_NUM_CONCURRENT_REQUESTS` across the platform. # Test: Search for the usage of `OC_DEVICE_MAX_NUM_CONCURRENT_REQUESTS`. Expect: Consistent usage across configuration files. rg --type c $'OC_DEVICE_MAX_NUM_CONCURRENT_REQUESTS'Length of output: 1166
port/android/ipadapter.c (4)
798-798: Ensure proper handling of wakeup pipe in network event thread.Verification successful
The
wakeup_pipeis properly initialized and used across the relevant functions in the codebase. The initialization, setting, reading, writing, and closing of thewakeup_pipeare all handled correctly.
- Initialization:
if (pipe(dev->wakeup_pipe) < 0)- Setting non-blocking:
if (!oc_fcntl_set_nonblocking(dev->wakeup_pipe[0]))- Usage in
FD_SETandFD_ISSET- Reading:
read(dev->wakeup_pipe[0], &buf, 1)- Writing:
write(dev->wakeup_pipe[1], "\n", 1)- Closing:
close(dev->wakeup_pipe[1])andclose(dev->wakeup_pipe[0])The handling of the
wakeup_pipein the network event thread appears to be correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the wakeup pipe is correctly initialized and used across all relevant functions. # Test: Search for the initialization and usage of `wakeup_pipe`. Expect: Proper initialization and error handling. rg --type python $'wakeup_pipe'Length of output: 61
Script:
#!/bin/bash # Description: Verify that the wakeup pipe is correctly initialized and used across all relevant functions. # Test: Search for the initialization and usage of `wakeup_pipe`. Expect: Proper initialization and error handling. rg --type c 'wakeup_pipe'Length of output: 1939
1375-1376: Check error handling for pipe creation.Verification successful
Check error handling for pipe creation.
The search results indicate that error handling for pipe creation is consistent across the codebase. The pattern of checking the return value of
pipeand logging an error message usingOC_ERRis followed in multiple instances.
port/linux/tcpadapter.c: if (pipe(dev->tcp.connect_pipe) < 0) { OC_ERR("Could not initialize connection pipe"); }port/linux/ipadapter.c: if (pipe(dev->wakeup_pipe) < 0) { OC_ERR("wakeup pipe: %d", errno); }port/esp32/adapter/src/ipadapter.c: if (vfs_pipe(dev->wakeup_pipe) < 0) { OC_ERR("wakeup pipe: %d", errno); }port/android/tcpadapter.c: if (pipe(dev->tcp.connect_pipe) < 0) { OC_ERR("Could not initialize connection pipe"); }port/android/ipadapter.c: if (pipe(dev->wakeup_pipe) < 0) { OC_ERR("wakeup pipe: %d", errno); }These instances confirm that the error handling for pipe creation is consistent and robust.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that error handling for pipe creation is consistent and robust across the codebase. # Test: Search for pipe creation and error handling patterns. Expect: Consistent and robust error handling. rg --type python $'pipe'Length of output: 54
Script:
#!/bin/bash # Description: Verify that error handling for pipe creation is consistent and robust across the codebase. # Test: Search for pipe creation and error handling patterns in C files. Expect: Consistent and robust error handling. rg --type c 'pipe'Length of output: 12016
1636-1637: Confirm resource release for wakeup pipes.Verification successful
The search results confirm that the wakeup pipes are being closed in multiple files, including
port/android/ipadapter.c. This indicates that the resources for the wakeup pipes are being released correctly.
port/linux/ipadapter.cport/esp32/adapter/src/ipadapter.cport/android/ipadapter.cScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that wakeup pipes are correctly closed and resources are released. # Test: Search for the closing of pipes and resource release. Expect: Correct closure and resource release. rg --type python $'close.*wakeup_pipe'Length of output: 70
Script:
#!/bin/bash # Description: Verify that wakeup pipes are correctly closed and resources are released. # Test: Search for the closing of pipes and resource release. Expect: Correct closure and resource release. rg --type c 'close.*wakeup_pipe'Length of output: 380
1613-1613: Ensure proper cleanup inoc_connectivity_shutdown.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- port/android/ipadapter.c (6 hunks)
- port/esp32/adapter/src/ipadapter.c (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- port/android/ipadapter.c
- port/esp32/adapter/src/ipadapter.c
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- port/linux/ipadapter.c (11 hunks)
Files skipped from review as they are similar to previous changes (1)
- port/linux/ipadapter.c
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
port/linux/ipadapter.c (1)
39-39: Consider adding a comment explaining why the inclusion of"port/oc_random.h"is necessary here, especially since it seems to be related to the new functionality for handling file descriptors randomly.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- port/android/ipadapter.c (6 hunks)
- port/esp32/adapter/src/ipadapter.c (6 hunks)
- port/linux/ipadapter.c (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- port/android/ipadapter.c
- port/esp32/adapter/src/ipadapter.c
Additional comments not posted (5)
port/linux/ipadapter.c (5)
1027-1037: The functionadd_control_flow_rfdsadds file descriptors for control flow to thefd_set. It's crucial that this function is called correctly to ensure that control messages are always processed, especially under high load or when the main event loop is busy with data sockets.
914-924: The functionfds_countiterates up tomax_fd, which is efficient. However, ensure thatmax_fdis always correctly updated throughout the code to avoid counting errors or performance issues.Verification successful
The
rgoutput shows several instances ofmax_fdinport/linux/ipadapter.c. To ensuremax_fdis always correctly updated, we need to review the context around each instance.
The context around each instance of
max_fdinport/linux/ipadapter.cshows thatmax_fdis being updated correctly within the functions. Specifically:
- In
fds_max,max_fdis updated within a loop to track the highest file descriptor set.- In
fds_count,max_fdis used as a parameter to limit the loop iteration.- In
pick_random_fd,max_fdis used to iterate through file descriptors.- In
remove_random_fds,max_fdis passed topick_random_fdto ensure correct iteration.These usages indicate that
max_fdis being managed appropriately within the code.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `max_fd` is always updated correctly. # Test: Search for updates to `max_fd` in the code. rg --type c "max_fd" port/linux/ipadapter.cLength of output: 420
Script:
#!/bin/bash # Description: Review the context around each instance of `max_fd` in port/linux/ipadapter.c # Extract lines around each instance of `max_fd` to understand its usage and updates rg --type c -C 5 "max_fd" port/linux/ipadapter.cLength of output: 1067
1702-1703: Closing the wakeup pipe inoc_connectivity_shutdownis a crucial cleanup step. Ensure that no further writes to the pipe occur after it is closed to avoid runtime errors.Verification successful
The
signal_event_threadfunction contains a write operation todev->wakeup_pipe[1]. We need to ensure that this function is not called after thewakeup_pipeis closed inoc_connectivity_shutdown.To verify this, we should check all calls to
signal_event_threadand ensure they occur before the pipe is closed.
The
signal_event_threadfunction is called withinoc_connectivity_shutdownbefore thewakeup_pipeis closed. This ensures that no writes to thewakeup_pipeoccur after it is closed, addressing the concern raised in the review comment.
signal_event_thread(dev)is called before thewakeup_pipeis closed inoc_connectivity_shutdown.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify no writes to wakeup pipe after closure. # Test: Search for writes to `wakeup_pipe` after its closure. rg --type c "write" port/linux/ipadapter.c | rg "wakeup_pipe"Length of output: 109
Script:
#!/bin/bash # Locate the oc_connectivity_shutdown function and examine the context around the write operation to wakeup_pipe. # Find the definition of oc_connectivity_shutdown function ast-grep --lang c --pattern 'void oc_connectivity_shutdown($$$) { $$$ }' # Find the context around the write operation to wakeup_pipe rg --context 10 'write(dev->wakeup_pipe[1], "\n", 1)' port/linux/ipadapter.cLength of output: 12836
Script:
#!/bin/bash # Locate the oc_connectivity_shutdown function and examine the context around the write operation to wakeup_pipe. # Find the definition of oc_connectivity_shutdown function ast-grep --lang c --pattern 'void oc_connectivity_shutdown($$$) { $$$ }' # Find the context around the write operation to wakeup_pipe with corrected regex rg --multiline --context 10 'write\(dev->wakeup_pipe\[1\], "\\n", 1\)' port/linux/ipadapter.cLength of output: 13148
Script:
#!/bin/bash # Find all calls to signal_event_thread and ensure they occur before the pipe is closed in oc_connectivity_shutdown. # Search for calls to signal_event_thread in port/linux/ipadapter.c rg --multiline --context 10 'signal_event_thread\(dev\)' port/linux/ipadapter.cLength of output: 788
895-900: The functionfd_sets_are_equaluses direct memory comparison to check if twofd_setstructures are equal. Ensure that this approach is portable and reliable across different systems and versions of the C library, as the internal representation offd_setmight vary.
681-694: The functionprocess_wakeup_signaleffectively clears the wakeup signal from the pipe. It's good to see proper handling ofEINTRduring the read operation. However, consider handling other potential errors fromread(other thanEINTR) to ensure robustness.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- port/android/ipadapter.c (6 hunks)
- port/esp32/adapter/src/ipadapter.c (6 hunks)
- port/linux/ipadapter.c (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- port/android/ipadapter.c
- port/esp32/adapter/src/ipadapter.c
Additional context used
Learnings (1)
port/linux/ipadapter.c (1)
User: Danielius1922 PR: iotivity/iotivity-lite#629 File: port/linux/ipadapter.c:926-939 Timestamp: 2024-06-05T08:47:11.485Z Learning: The Arduino port is currently broken and should be ignored in analyses and suggestions.
Additional comments not posted (1)
port/linux/ipadapter.c (1)
926-939: Verify random number generation seeding.The function
pick_random_fdusesoc_random_value()for generating a random file descriptor index. Ensure that the random number generator is adequately seeded to prevent patterns or predictability, especially since this function is critical for load distribution or random drop of requests under high load.Verification successful
The implementation of
oc_random_valueinport/linux/random.creads from/dev/urandom, which is a secure source of random numbers on Linux systems. This method does not require explicit seeding as/dev/urandomis already seeded by the operating system.
- The function
oc_random_initopens/dev/urandomfor reading.- The function
oc_random_valuereads random values from/dev/urandom.No explicit seeding functions (
srand,seed48,srandom,initstate) are necessary or found inport/linux/random.c.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the seeding of random number generation. # Test: Search for the seeding of the random number generator. rg --type c "srand|seed48|srandom|initstate" port/linux/ipadapter.cLength of output: 69
Script:
#!/bin/bash # Description: Verify the implementation and seeding of the random number generator. # Search for the definition of oc_random_value rg --type c "oc_random_value" # Search for seeding functions in the entire codebase rg --type c "srand|seed48|srandom|initstate"Length of output: 1772
Script:
#!/bin/bash # Description: Verify the implementation and seeding of the random number generator in port/linux/random.c. # Extract the implementation of oc_random_value in port/linux/random.c rg --type c --context 10 "oc_random_value" port/linux/random.c # Check for seeding functions in port/linux/random.c rg --type c "srand|seed48|srandom|initstate" port/linux/random.cLength of output: 493
… in Embedded Devices.
- signal when the event queue is ready to process events again. - separate processing of control flow and read file desriptors - random pick slection and processing of network event
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- port/android/ipadapter.c (7 hunks)
- port/esp32/adapter/src/ipadapter.c (6 hunks)
- port/linux/ipadapter.c (11 hunks)
Files not summarized due to errors (1)
- port/linux/ipadapter.c: Error: Server error. Please try again later.
Files not reviewed due to errors (1)
- port/linux/ipadapter.c (no review received)
Additional comments not posted (25)
port/android/ipadapter.c (15)
41-41: Includeport/oc_random.hto support new functionalities related to random operations.
785-787: Ensure correct comparison offd_setstructures usingmemcmp.
790-800: Optimize thefds_maxfunction by reversing the loop to find the maximum file descriptor more efficiently.
802-812: Consider optimizingfds_countby using a reverse loop for efficiency.
815-825: Ensure proper random selection of file descriptors with error handling for empty sets.
827-842: Ensure robust removal of random file descriptors, handling cases where removal is not possible.
845-857: Ensureadd_control_flow_rfdscorrectly handles all network interfaces.
877-891: Ensure proper handling of the wakeup signal with robust error checking.
893-1004: Ensure efficient and error-resilient handling of socket read events, including proper cleanup on failure.
1006-1037: Ensure proper event processing with debug logging for unhandled events.
1039-1090: Ensure comprehensive event processing including control flow and dynamic allocation considerations.
1092-1138: Ensure the network event thread properly manages file descriptor sets and handles termination correctly.
1762-1778: Ensure robust signaling to the network thread, handling all potential write errors correctly.
1781-1792: Ensure that the wakeup functionality correctly handles the absence of an IP context.
Line range hint
1793-1828: Ensure clean shutdown of connectivity resources, properly closing all file descriptors and freeing memory.port/esp32/adapter/src/ipadapter.c (10)
30-30: The inclusion of"port/oc_random.h"is appropriate given the need for randomized operations in managing file descriptors.
808-867: The functions for managing file descriptors (fd_sets_are_equal,fds_max,fds_count,pick_random_fd,remove_random_fds) are well-implemented. These are crucial for the dynamic management of network events and align with the PR's objectives to handle memory constraints on embedded devices.
870-897: The functionprocess_wakeup_signalefficiently clears the wakeup signal from the file descriptor set, ensuring the network thread can continue processing other events. This is a good use of non-blocking I/O operations.
899-930: The functionprocess_socket_read_eventeffectively processes incoming network events. It's important to ensure that memory allocated withoc_allocate_message()is always freed, as seen here, to avoid memory leaks.
965-1001: Theprocess_eventsfunction appears to handle control flow and dynamic allocation correctly, reducing the number of processed events when the queue is full. This is crucial for preventing resource exhaustion under high load, as described in the PR.
1003-1049: Thenetwork_event_threadfunction correctly initializes and continually updates the file descriptor sets and handles network events based on current system state. The dynamic allocation checks and control flow adjustments are well-integrated.
1517-1518: The error handling in thevfs_pipesetup is crucial for robustness. The log message provides clear error reporting. This change was previously reviewed and accepted.
1807-1808: The error handling for thecloseoperations on wakeup pipes ensures resources are properly released and errors are logged, increasing the robustness of the shutdown process. This change was previously reviewed and accepted.
1742-1758: Thesignal_event_threadfunction properly handles interruptions and logs any issues encountered during the signaling process. This is critical for maintaining reliable inter-thread communication.
1760-1772: Theoc_connectivity_wakeupfunction is designed to safely handle wake-up signals across different devices. This is part of the dynamic allocation feature and is implemented correctly.
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
port/linux/ipadapter.c (1)
Line range hint
1626-1678: The functionsignal_event_threadis correctly implemented to signal the network event thread. However, consider adding a debug log for successful signaling for better traceability.+ OC_DBG("Signaling network thread: write result=%zd", result);port/esp32/adapter/src/ipadapter.c (1)
937-968: In theprocess_eventfunction, the handling of socket read events is concise. The debug logging for unhandled file descriptors in theOC_DBG_IS_ENABLEDblock provides good diagnostics support. Ensure that this debug functionality does not introduce performance overhead in production builds.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- port/android/ipadapter.c (7 hunks)
- port/esp32/adapter/src/ipadapter.c (6 hunks)
- port/linux/ipadapter.c (11 hunks)
Additional comments not posted (20)
port/linux/ipadapter.c (6)
39-39: Include"port/oc_random.h"to support random operations.
681-694: Ensure that the return value fromreadis correctly handled to avoid infinite loops or undefined behavior if the pipe is unexpectedly closed.
895-910: The logic for managing file descriptors and their sets using dynamic allocation has been implemented correctly.Also applies to: 912-922, 924-934, 936-952, 954-969
1041-1051: Refactor to improve readability inadd_control_flow_rfds.
1535-1540: Ensure that the non-blocking mode is set correctly forwakeup_pipe[0]to avoid potential blocking operations that could affect system responsiveness.
1670-1678: The functionoc_connectivity_wakeupcorrectly handles the wake-up signaling for network threads. It properly checks for null pointers and logs warnings appropriately.port/android/ipadapter.c (6)
41-41: The inclusion ofoc_random.his appropriate for the new functionalities that involve random operations.
882-896: Theprocess_wakeup_signalfunction correctly handles the wakeup signal, ensuring robust control flow. Good use of non-blocking reads to handle EINTR errors.
1011-1042: Theprocess_eventandprocess_eventsfunctions effectively orchestrate event handling, ensuring that control flow and socket events are managed efficiently. The dynamic allocation checks and handling of control flow file descriptors are particularly well-implemented.Also applies to: 1044-1095
1097-1141: Thenetwork_event_threadfunction is robust, ensuring that all network events are processed efficiently. The use ofselectto manage file descriptors and the handling of termination conditions are well-done. Ensure that the thread safety and error handling are thoroughly tested, especially in edge cases.
Line range hint
1798-1833: Theoc_connectivity_shutdownfunction is thorough, ensuring that all sockets are closed and all resources are freed. The sequential handling of different components (IPv4, security, TCP) is logical and ensures a clean shutdown.
850-862: Theadd_control_flow_rfdsfunction correctly adds network interface monitoring on the 0th device, aligning with the specified design. Theudp_add_socks_to_rfd_setfunction effectively manages UDP sockets. Ensure all relevant network interfaces are considered if extending beyond the 0th device.Also applies to: 864-880
Verification successful
The context around the
add_control_flow_rfdsfunction calls in the identified files shows that it is used consistently across different platforms (linux, esp32, android). The function is designed to monitor network interface changes on the 0th logical device and add control flow file descriptors to the providedfd_set.The review comment's concern about ensuring all relevant network interfaces are considered if extending beyond the 0th device seems valid. However, the current implementation clearly restricts monitoring to the 0th device, which appears to be an intentional design choice.
Given this, the review comment's suggestion to ensure all relevant network interfaces are considered is more of a future enhancement rather than a current issue.
- port/linux/ipadapter.c
- port/esp32/adapter/src/ipadapter.c
- port/android/ipadapter.c
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `add_control_flow_rfds` handles all network interfaces correctly. # Test: Search for the function usage. Expect: Correct handling of all network interfaces. rg --type c 'add_control_flow_rfds'Length of output: 737
Script:
#!/bin/bash # Extract context around the `add_control_flow_rfds` function calls in the identified files for review. rg --type c -C 5 'add_control_flow_rfds' port/linux/ipadapter.c rg --type c -C 5 'add_control_flow_rfds' port/esp32/adapter/src/ipadapter.c rg --type c -C 5 'add_control_flow_rfds' port/android/ipadapter.cLength of output: 2919
port/esp32/adapter/src/ipadapter.c (8)
875-889: The functionprocess_wakeup_signaleffectively checks and clears the wakeup signal. It's good to see the use of a do-while loop to handleEINTRerrors robustly. This is crucial for maintaining the responsiveness of the network event handling thread.
970-1006: Theprocess_eventsfunction orchestrates the handling of multiple network events. The dynamic allocation logic from lines 987-996, which adjusts the number of file descriptors based on the queue's capacity, is a critical part of preventing message queue overflow. This is a key enhancement for resilience against DDOS attacks.
1522-1523: The error handling for thevfs_pipecreation is correctly implemented. The previous comments and the changes made bysp-martinhave addressed this adequately.
1747-1763: The functionsignal_event_threadcorrectly handles the signaling of the network event thread. The loop to handleEINTRand the check forENOSPCare well done, ensuring robust signal handling.
1765-1776: Theoc_connectivity_wakeupfunction is a good abstraction for waking up the network thread associated with a specific device. This encapsulation enhances modularity and reusability of the wakeup mechanism.
1812-1813: Ensure proper error handling forcloseoperations on the wakeup pipe. This is crucial for resource management and preventing resource leaks during shutdown.
891-902: The functionadd_control_flow_rfdsadds file descriptors for control flow. It's important to ensure thatifchange_sockis properly managed elsewhere in the code since it's commented out here but might be critical for handling interface changes.Verification successful
ifchange_sockis not actively used in the ESP32 codebase as all related code is commented out.
port/esp32/adapter/src/ipadapter.c: All instances ofifchange_sockare commented out, including its declaration, usage, and cleanup.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify management of `ifchange_sock` across the codebase. # Test: Search for the usage of `ifchange_sock`. Expect: Proper initialization and cleanup. rg --type c "ifchange_sock"Length of output: 2406
Script:
#!/bin/bash # Description: Verify management of `ifchange_sock` specifically in ESP32 codebase. # Test: Search for initialization, usage, and cleanup of `ifchange_sock` in ESP32 code. rg --type c "ifchange_sock" port/esp32/adapter/src/Length of output: 244
904-935: The functionprocess_socket_read_eventhandles the reading of messages from sockets efficiently. The use ofoc_udp_receive_messageand conditionallyoc_tcp_receive_message(guarded byOC_TCP) is well-structured. However, ensure that the memory allocated formessagein line 907 is always freed correctly to avoid memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/plgd-device-tests.yml (1 hunks)
- .github/workflows/plgd-hub-tests.yml (1 hunks)
- api/oc_network_events.c (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/plgd-device-tests.yml
- .github/workflows/plgd-hub-tests.yml
Additional comments not posted (5)
api/oc_network_events.c (5)
48-60: The implementation ofget_events_queue_lengthis correct and efficiently counts messages per device.
62-76: The logic insend_wakeup_signalto manage wakeup signals based on message queue length is correctly implemented.
99-103: Integration ofsend_wakeup_signalinoc_process_network_eventcorrectly manages potential queue overflow scenarios.
Line range hint
192-230: The logic inoc_network_drop_receive_eventsfor managing message dropping and conditional wakeup signaling based on queue status is correctly implemented.
257-266: The thread-safe implementation ofoc_network_get_event_queue_lengthis correct and aligns with best practices for concurrent programming.
|
Danielius1922
added
OCF Conformance Testing
SiMet
added
OCF Conformance Testing
|
I see tests passed: Other job was working and block this run. Hope now will be perfomed without any interuptions |
Description
The current network thread can produce unlimited messages to the queue which is consumed by the event thread. In embedded devices, we have limited memory, so a DDOS attack could exhaust the memory.
Fixes #626
Implementation details
To fix this issue, we use the macro OC_MAX_NUM_CONCURRENT_REQUESTS along with setting OC_DYNAMIC_ALLOCATION, which defines the maximum number of messages in the queue per device. The default value of OC_MAX_NUM_CONCURRENT_REQUESTS is 32 for OC_DYNAMIC_ALLOCATION.
If the number of messages reaches the limit, all file descriptors in the network thread that produce oc_message_t are removed from select/poll. The file descriptor for monitoring the control flow is always set and is used to wake up from select() when the queue is ready to accept events again.
When more file descriptors are ready to be read than the number of available queue slots, a subset is randomly selected to be pushed to the queue.