unwrap cleanup in types.rs #1122

yue-fred-gao · 2025-12-01T19:11:17Z

why I did it

unwrap() and expect function may crash at runtime. For the issues that may happen, we should catch the error and handle it gracefully instead of crashing.

This PR focuses unwrap calls in types.rs

How I did it

Return error if it fails to convert c-type string to rust str. For the case of take-style functions, such as take_cstr, it will continue the operation and free the memory then return the error. This is to avoid memory leak. Ideally, the functions should act atomically. If it fails, it leaves input intact. However, it requires copy memory or scan twice, which is not efficient for some very rare error and the caller needs to free the memory anyway.

Signed-off-by: Yue Gao <[email protected]>

- take_cstr - take_string_array - take_field_value_array - take_key_op_field_values_array Signed-off-by: Yue Gao <[email protected]>

- from make_field_value_array - from make_key_op_field_values_array Signed-off-by: Yue Gao <[email protected]>

Signed-off-by: Yue Gao <[email protected]>

mssonicbld · 2025-12-01T19:11:25Z

/azp run

azure-pipelines · 2025-12-01T19:11:36Z

Azure Pipelines successfully started running 1 pipeline(s).

yue-fred-gao · 2025-12-01T22:11:45Z

hi @qiluo-msft and @saiarcot895, please review

Copilot

Pull request overview

This PR systematically removes unwrap() and expect() calls from types.rs and propagates error handling changes throughout the codebase. The changes convert C string operations to return Result types, enabling graceful error handling instead of runtime panics when encountering null bytes or invalid UTF-8 sequences.

Key Changes:

Modified cstr() and take_cstr() to return Result types with descriptive error messages
Updated take_field_value_array(), take_key_op_field_values_array(), and take_string_array() to handle conversion errors while ensuring all C memory is freed
Propagated ? operator changes to ~80 call sites across 14 files

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
crates/swss-common/src/types.rs	Core changes: converted string conversion functions to return Result, updated array processing functions with error handling and memory cleanup
crates/swss-common/src/types/zmqserver.rs	Propagated error handling for endpoint parameter
crates/swss-common/src/types/zmqproducerstatetable.rs	Updated table_name, key parameters and field value array creation to use ? operator
crates/swss-common/src/types/zmqconsumerstatetable.rs	Updated table_name parameter and pops() return to propagate errors
crates/swss-common/src/types/zmqclient.rs	Updated endpoint, db_name, table_name parameters and array creation to use ? operator
crates/swss-common/src/types/table.rs	Updated key, field parameters and array functions to propagate errors
crates/swss-common/src/types/subscriberstatetable.rs	Updated table_name parameter and pops() return to propagate errors
crates/swss-common/src/types/sonicv2connector.rs	Updated multiple string parameters with good use of .transpose() for Optional parameters
crates/swss-common/src/types/producerstatetable.rs	Updated table_name, key parameters and field value array creation to use ? operator
crates/swss-common/src/types/logger.rs	Updated db_name and hardcoded string literals to use ? operator
crates/swss-common/src/types/events.rs	Updated event_source and event_tag parameters; good use of .collect::<Result<Vec<_>>>() pattern
crates/swss-common/src/types/dbconnector.rs	Updated connection parameters (hostname, sock_path, db_name, etc.) and data access methods to propagate errors
crates/swss-common/src/types/consumerstatetable.rs	Updated table_name parameter and pops() return to propagate errors
crates/swss-common/src/types/configdbconnector.rs	Updated netns, table, key parameters and array functions to propagate errors
crates/swss-common/src/lib.rs	Updated path parameters in config initialization functions to use ? operator

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-01T23:10:19Z

crates/swss-common/src/types.rs

+                    err = Some(e);
+                    break;


Memory leak: When take_cstr fails during iteration, it frees the current string s but doesn't free the remaining strings in the array. The break statement exits the loop early, leaving unprocessed strings that are never freed before SWSSStringArray_free(arr) is called.

To fix this, continue iterating through all strings to free them even after an error occurs, similar to the pattern used in take_field_value_array.

Suggested change

err = Some(e);

break;

// Record the first error, if any

if err.is_none() {

err = Some(e);

}

// Continue to next entry to ensure all C resources are freed

``` #Closed

This is a valid Memory leak bug.

Copilot · 2025-12-01T23:10:19Z

crates/swss-common/src/types.rs

    let arr = SWSSFieldValueArray {
        data: data.as_mut_ptr(),
-        len: data.len().try_into().unwrap(),
+        len: data.len().try_into().map_err(|_| Exception::new("field value array length doesn't fit target type".to_string()))?,


The error message "field value array length doesn't fit target type" is vague. Consider making it more specific by indicating what the actual length was and what the target type's maximum is, e.g., format!("field value array length {} exceeds maximum for target type", data.len())

Copilot · 2025-12-01T23:10:19Z

crates/swss-common/src/types.rs

    let arr = SWSSKeyOpFieldValuesArray {
        data: data.as_mut_ptr(),
-        len: data.len().try_into().unwrap(),
+        len: data.len().try_into().map_err(|_| Exception::new("key-op field values array length doesn't fit target type".to_string()))?,


The error message "key-op field values array length doesn't fit target type" is vague. Consider making it more specific by indicating what the actual length was and what the target type's maximum is, e.g., format!("key-op field values array length {} exceeds maximum for target type", data.len())

Suggested change

len: data.len().try_into().map_err(|_| Exception::new("key-op field values array length doesn't fit target type".to_string()))?,

len: data.len().try_into().map_err(|_| Exception::new(format!(

"key-op field values array length {} exceeds maximum for target type (max {})",

data.len(),

usize::MAX

)))?,

Copilot · 2025-12-01T23:10:20Z

crates/swss-common/src/types.rs

+pub(crate) fn cstr(s: impl AsRef<[u8]>) -> Result<CString> {
+    CString::new(s.as_ref())
+        .map_err(|e| Exception::new(format!("String contains null byte at position {}", e.nul_position())))
 }

 /// Take a malloc'd c string and convert it to a native String
-pub(crate) unsafe fn take_cstr(p: *const libc::c_char) -> String {
-    let s = CStr::from_ptr(p)
-        .to_str()
-        .expect("C string being converted to Rust String contains invalid UTF-8")
-        .to_string();
+/// If any string fails to convert, returns an error and frees all allocated memory.
+pub(crate) unsafe fn take_cstr(p: *const libc::c_char) -> Result<String> {
+    let cstr = CStr::from_ptr(p);
+    // Convert to Rust String, capturing UTF-8 conversion errors.
+    let result: Result<String> = match cstr.to_str() {
+        Ok(s) => Ok(s.to_string()),
+        Err(_) => Err(Exception::new("C string being converted to Rust String contains invalid UTF-8".to_string())),
+    };
+
+    // Free the original C string in all cases to avoid leaks.
    libc::free(p as *mut libc::c_void);
-    s
+
+    result
 }


The new error handling in cstr() for null bytes and in take_cstr() for invalid UTF-8 lacks test coverage. Consider adding tests that verify:

cstr() correctly returns an error when given a string containing a null byte

take_cstr() correctly handles and returns an error for invalid UTF-8 sequences while still freeing the C string

The take_* array functions correctly handle errors while freeing all allocated memory

These error paths are now part of the API contract and should be tested to prevent regressions.

crates/swss-common/src/types.rs

Signed-off-by: Yue Gao <[email protected]>

mssonicbld · 2025-12-02T16:03:05Z

/azp run

azure-pipelines · 2025-12-02T16:03:19Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-12-02T16:29:29Z

/azp run

azure-pipelines · 2025-12-02T16:29:42Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-12-03T22:55:39Z

/azp run

azure-pipelines · 2025-12-03T22:55:50Z

Azure Pipelines successfully started running 1 pipeline(s).

yue-fred-gao · 2025-12-04T17:52:52Z

/azpw run

mssonicbld · 2025-12-04T17:52:55Z

/AzurePipelines run

azure-pipelines · 2025-12-04T17:53:09Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-12-10T15:33:40Z

/azp run

azure-pipelines · 2025-12-10T15:33:56Z

Azure Pipelines successfully started running 1 pipeline(s).

yue-fred-gao added 5 commits December 1, 2025 13:58

Remove unwrap and expect in swss-common types.rs

f5ba0f6

Signed-off-by: Yue Gao <[email protected]>

Finish changing cstr to return Result

7b33055

Signed-off-by: Yue Gao <[email protected]>

Remove unwrap from below functions

8a24302

- take_cstr - take_string_array - take_field_value_array - take_key_op_field_values_array Signed-off-by: Yue Gao <[email protected]>

Remove unwraps

eb6945e

- from make_field_value_array - from make_key_op_field_values_array Signed-off-by: Yue Gao <[email protected]>

Undo change to configure.ac

6dfe77d

Signed-off-by: Yue Gao <[email protected]>

qiluo-msft requested a review from Copilot December 1, 2025 23:06

Copilot started reviewing on behalf of qiluo-msft December 1, 2025 23:07 View session

Copilot finished reviewing on behalf of qiluo-msft December 1, 2025 23:09

Copilot AI reviewed Dec 1, 2025

View reviewed changes

qiluo-msft reviewed Dec 1, 2025

View reviewed changes

crates/swss-common/src/types.rs Show resolved Hide resolved

Address review comments

9ae8a0b

Signed-off-by: Yue Gao <[email protected]>

Merge branch 'sonic-net:master' into types_unwrap_cleanup

2a767f7

qiluo-msft approved these changes Dec 2, 2025

View reviewed changes

Merge branch 'sonic-net:master' into types_unwrap_cleanup

9ff74b4

Merge branch 'sonic-net:master' into types_unwrap_cleanup

33f7c49

qiluo-msft merged commit 3df0382 into sonic-net:master Dec 29, 2025
19 checks passed

-                    err = Some(e);
-                    break;
+                    // Record the first error, if any
+                    if err.is_none() {
+                        err = Some(e);
+                    }
+                    // Continue to next entry to ensure all C resources are freed
+``` #Closed

-        len: data.len().try_into().map_err(|_| Exception::new("key-op field values array length doesn't fit target type".to_string()))?,
+        len: data.len().try_into().map_err(|_| Exception::new(format!(
+            "key-op field values array length {} exceeds maximum for target type (max {})",
+            data.len(),
+            usize::MAX
+        )))?,

unwrap cleanup in types.rs #1122

unwrap cleanup in types.rs #1122

Uh oh!

Conversation

yue-fred-gao commented Dec 1, 2025

why I did it

How I did it

Uh oh!

mssonicbld commented Dec 1, 2025

Uh oh!

azure-pipelines bot commented Dec 1, 2025

Uh oh!

yue-fred-gao commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 1, 2025 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mssonicbld commented Dec 2, 2025

Uh oh!

azure-pipelines bot commented Dec 2, 2025

Uh oh!

mssonicbld commented Dec 2, 2025

Uh oh!

azure-pipelines bot commented Dec 2, 2025

Uh oh!

mssonicbld commented Dec 3, 2025

Uh oh!

azure-pipelines bot commented Dec 3, 2025

Uh oh!

yue-fred-gao commented Dec 4, 2025

Uh oh!

mssonicbld commented Dec 4, 2025

Uh oh!

azure-pipelines bot commented Dec 4, 2025

Uh oh!

mssonicbld commented Dec 10, 2025

Uh oh!

azure-pipelines bot commented Dec 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI Dec 1, 2025 •

edited by qiluo-msft

Loading