Fix risky unwrap(), expect(), and casting #1113

qiluo-msft · 2025-11-25T00:44:01Z

unwrap() function may crash in runtime.
Drop should not crash.
Clone should not crash, so remove it.
Protect casting Duration

Depends on sonic-net/sonic-dash-ha#132

Signed-off-by: Qi Luo <[email protected]>

mssonicbld · 2025-11-25T00:44:09Z

/azp run

azure-pipelines · 2025-11-25T00:44:21Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR improves Rust code safety by removing risky operations (unwrap(), expect()) and unnecessary type casting. The changes span both Rust FFI bindings and C API implementations, changing file descriptor types from uint32_t to int32_t to match the underlying C++ int getFd() return type, and replacing Duration parameters with u32 timeout_ms for more direct control.

Changed file descriptor types from uint32_t to int32_t in C API to match underlying C++ implementation
Removed unwrap() calls when creating BorrowedFd since type casting is no longer needed
Replaced expect() calls in Drop implementations with eprintln! to prevent panics during cleanup
Changed read_data() API from accepting Duration to u32 timeout_ms (breaking change)
Removed Clone trait implementation for DbConnector to prevent potential panics

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/c_api_ut.cpp	Updated test file descriptors from uint32_t to int32_t
common/c-api/consumerstatetable.h	Changed getFd signature to return int32_t instead of uint32_t
common/c-api/consumerstatetable.cpp	Removed numeric_cast from getFd implementation
common/c-api/subscriberstatetable.h	Changed getFd signature to return int32_t instead of uint32_t
common/c-api/subscriberstatetable.cpp	Removed numeric_cast from getFd implementation
common/c-api/zmqconsumerstatetable.h	Changed getFd signature to return int32_t instead of uint32_t
common/c-api/zmqconsumerstatetable.cpp	Removed numeric_cast from getFd implementation
crates/swss-common/src/types/consumerstatetable.rs	Removed Duration import, unwrap() call, and updated Drop to use eprintln!
crates/swss-common/src/types/subscriberstatetable.rs	Removed Duration import, unwrap() call, and updated Drop to use eprintln!
crates/swss-common/src/types/zmqconsumerstatetable.rs	Removed Duration import, unwrap() call, updated Drop to use eprintln!, and improved comment capitalization
crates/swss-common/src/types/table.rs	Updated Drop implementation to use eprintln! instead of expect()
crates/swss-common/src/types/producerstatetable.rs	Updated Drop implementation to use eprintln! instead of expect()
crates/swss-common/src/types/zmqclient.rs	Updated Drop implementation to use eprintln! instead of expect()
crates/swss-common/src/types/dbconnector.rs	Removed Clone trait implementation and updated Drop to use eprintln!

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

mssonicbld · 2025-11-25T01:14:23Z

/azp run

azure-pipelines · 2025-11-25T01:14:35Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-25T01:36:56Z

/azp run

azure-pipelines · 2025-11-25T01:37:08Z

Azure Pipelines successfully started running 1 pipeline(s).

saiarcot895 · 2025-11-25T03:22:29Z

crates/swss-common/src/types/zmqconsumerstatetable.rs


-    pub fn read_data(&self, timeout: Duration, interrupt_on_signal: bool) -> Result<SelectResult> {
-        let timeout_ms = timeout.as_millis().try_into().unwrap();
+    pub fn read_data(&self, timeout_ms: u32, interrupt_on_signal: bool) -> Result<SelectResult> {


The reason that Duration is used is to signify to the caller that this argument is a time argument, and so something time-based should be passed in. C language not having a discrete time type shouldn't be a reason not to use it in Rust code. #Resolved

I would instead recommend handling a duration that is too large gracefully: return an error saying the duration was too big (or similar).

resolved. and also change return type.

saiarcot895 · 2025-11-25T03:30:22Z

crates/swss-common/src/types/subscriberstatetable.rs

+        unsafe {
+            if let Err(e) = swss_try!(SWSSSubscriberStateTable_free(self.ptr)) {
+                eprintln!("Error dropping SubscriberStateTable: {}", e);
+            }


What does it mean for a *_free function to fail? The object got corrupted? I'm not sure if just printing on stderr and continuing the application is safe at that point. #Resolved

It may be possible that a programmer mistake of throwing in dtor in this class or any base class or any member class. Crashing a critical daemon is not good for online service, so use eprintln as defensive programming programming.

Running after an exception is thrown in dtor is not good either; the runtime state of the program is no longer known or consistent.

I plan to fix exception is thrown in dtor issue in another future PR.

The future PR is #1115

yue-fred-gao · 2025-11-25T14:42:27Z

crates/swss-common/src/types/consumerstatetable.rs

        unsafe {
            let fd = swss_try!(p_fd => SWSSConsumerStateTable_getFd(self.ptr, p_fd))?;
-            let fd = BorrowedFd::borrow_raw(fd.try_into().unwrap());
+            let fd = BorrowedFd::borrow_raw(fd);


borrow_raw document says "The resource pointed to by fd must remain open for the duration of the returned BorrowedFd, and it must not have the value -1.". We should check the value is not negative. #Resolved

checked for -1, not for negative. Since BorrowedFd and hiredis cares only about -1.

yue-fred-gao · 2025-11-25T14:44:58Z

crates/swss-common/src/types/consumerstatetable.rs


-    pub fn read_data(&self, timeout: Duration, interrupt_on_signal: bool) -> Result<SelectResult> {
-        let timeout_ms = timeout.as_millis().try_into().unwrap();
+    pub fn read_data(&self, timeout_ms: u32, interrupt_on_signal: bool) -> Result<SelectResult> {


Instead of changing the argument to u32, you can timeout.as_millis().try_into().map_err(|_| Error::new("Timeout value too large to convert"))? #Resolved

resolved. and also change return type.

mssonicbld · 2025-11-25T23:05:25Z

/azp run

azure-pipelines · 2025-11-25T23:05:36Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-11-25T23:59:19Z

/azp run

azure-pipelines · 2025-11-25T23:59:30Z

Azure Pipelines successfully started running 1 pipeline(s).

crates/swss-common/src/types/dbconnector.rs

saiarcot895 · 2025-11-26T01:05:39Z

crates/swss-common/tests/sync.rs

    let redis = Redis::start();
-    let pst = ProducerStateTable::new(redis.db_connector(), "table_a")?;
-    let cst = ConsumerStateTable::new(redis.db_connector(), "table_a", None, None)?;
+    let pst = ProducerStateTable::new(redis.db_connector(), "table_a").unwrap();


Why unwrap() on test code? The old code will already return an error, and the test will be gracefully marked as failed. #Resolved

The challenge is that read_data, and other function? will return different Result. So it is easy to unwrap and let testcase fail. The failure message is also kind of graceful

thread 'consumer_producer_state_tables_sync_api_basic_test' panicked at crates/swss-common/tests/sync.rs:76:12: called `Option::unwrap()` on a `None` value test result: FAILED. 7 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.43s error: test failed, to rerun pass `-p swss-common --test sync`

what's the reason to change return type of read_data? Is it really necessary. It seems a lot of churn due to that

The reason is that we want to use Duration instead of u32, and the casting error handling introduce new Result type. Similar change also happens on get_fd().

pub fn read_data(&self, timeout: Duration, interrupt_on_signal: bool) -> std::io::Result<SelectResult>

Another option is to keep both function as thin wrapper of c-api and keep the most native data types, then we can keep the Result type.

We can keep the Result type, which is

sonic-swss-common/crates/swss-common/src/types/exception.rs

Line 8 in e67092c

pub type Result<T, E = Exception> = std::result::Result<T, E>;

. You can add a constructor to Exception then you can use it in map_err.

mssonicbld · 2025-11-26T20:52:18Z

/azp run

azure-pipelines · 2025-11-26T20:52:32Z

Azure Pipelines successfully started running 1 pipeline(s).

liushilongbuaa · 2025-12-01T03:15:01Z

@qiluo-msft , It seems this PR breaks official build.
Why we didn't update submodule ref but it takes this commit?

This reverts commit d6ccff7.

yue-fred-gao · 2025-12-01T14:42:49Z

@qiluo-msft , It seems this PR breaks official build. Why we didn't update submodule ref but it takes this commit?

I think this is because sonic-dash-ha and all rust components are pulling swss-common master. See https://github.com/sonic-net/sonic-dash-ha/blob/b38d8fb68c7e1a1d3c866fdb65e840c7da1021d8/Cargo.toml#L100. sonic-dash-ha has been updated to accommodate the change in swss-common. We need to update its ref in sonic-buildimage.

yue-fred-gao · 2025-12-01T15:00:12Z

@qiluo-msft , It seems this PR breaks official build. Why we didn't update submodule ref but it takes this commit?

I saw the error in sonic-buildimage. The issue is that libswsscommon is built from src/sonic-swss-common mounted by sonic-buildimage but rust is pulled directly from swss-common master branch. libswsscommon lags behind rust swss-common crate and causes the compilation error. We need to update ref of swss-common in sonic-buildimage to the latest.

Going forward, probably we should change the way rust components pulling swss-common. Instead of always from the latest of the branch, they can use relative path to find swss-common in sonic-buildimage. When we cut a new branch, we don't need to update cargo.toml to the new branch (otherwise, they always use swss-common master branch).

yue-fred-gao · 2025-12-01T16:13:53Z

@qiluo-msft , I submitted PR sonic-net/sonic-buildimage#24663 to fix the issue. And let's discuss the long term fix I proposed above.

This reverts commit d6ccff7.

Remove risky unwrap(), expect(), and casting

895005c

Signed-off-by: Qi Luo <[email protected]>

qiluo-msft requested review from r12f, saiarcot895 and yue-fred-gao November 25, 2025 00:44

qiluo-msft marked this pull request as ready for review November 25, 2025 00:46

Copilot AI review requested due to automatic review settings November 25, 2025 00:46

Copilot started reviewing on behalf of qiluo-msft November 25, 2025 00:47 View session

Copilot finished reviewing on behalf of qiluo-msft November 25, 2025 00:49

Copilot AI reviewed Nov 25, 2025

View reviewed changes

qiluo-msft marked this pull request as draft November 25, 2025 01:06

Fix test code

f69a4b5

Fix type errors

7e6ef5a

saiarcot895 reviewed Nov 25, 2025

View reviewed changes

yue-fred-gao reviewed Nov 25, 2025

View reviewed changes

get_fd should check invalid fd

3c3e186

timeout is type of Duration

3dceea4

qiluo-msft changed the title ~~Remove risky unwrap(), expect(), and casting~~ Fix risky unwrap(), expect(), and casting Nov 26, 2025

qiluo-msft marked this pull request as ready for review November 26, 2025 00:07

saiarcot895 reviewed Nov 26, 2025

View reviewed changes

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

saiarcot895 reviewed Nov 26, 2025

View reviewed changes

Add new() into Exception class

c5d7c8f

yue-fred-gao approved these changes Nov 26, 2025

View reviewed changes

saiarcot895 approved these changes Nov 26, 2025

View reviewed changes

qiluo-msft merged commit d6ccff7 into sonic-net:master Nov 29, 2025
18 checks passed

liushilongbuaa added a commit that referenced this pull request Dec 1, 2025

Revert "Fix risky unwrap(), expect(), and casting (#1113)"

1f4a1cc

This reverts commit d6ccff7.

liushilongbuaa mentioned this pull request Dec 1, 2025

Revert "Fix risky unwrap(), expect(), and casting" #1118

Open

AnantKishorSharma mentioned this pull request Dec 1, 2025

[dhcp_relay] sonic dhcp relay agent for IPv4 sonic-net/sonic-buildimage#22486

Open

9 tasks

yue-fred-gao mentioned this pull request Dec 1, 2025

Fixes #24661: Update reference to sonic-dash-ha and sonic-swss-common sonic-net/sonic-buildimage#24663

Open

9 tasks

qiluo-msft deleted the qiluo/unwrap branch December 1, 2025 17:37

dprital added a commit to dprital/sonic-swss-common that referenced this pull request Dec 1, 2025

Revert "Fix risky unwrap(), expect(), and casting (sonic-net#1113)"

0504dc5

This reverts commit d6ccff7.

Fix risky unwrap(), expect(), and casting #1113

Fix risky unwrap(), expect(), and casting #1113

Conversation

qiluo-msft commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Nov 25, 2025

Uh oh!

azure-pipelines bot commented Nov 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mssonicbld commented Nov 25, 2025

Uh oh!

azure-pipelines bot commented Nov 25, 2025

Uh oh!

mssonicbld commented Nov 25, 2025

Uh oh!

azure-pipelines bot commented Nov 25, 2025

Uh oh!

saiarcot895 Nov 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saiarcot895 Nov 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yue-fred-gao Nov 25, 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!

Choose a reason for hiding this comment

Uh oh!

yue-fred-gao Nov 25, 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!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Nov 25, 2025

Uh oh!

azure-pipelines bot commented Nov 25, 2025

Uh oh!

mssonicbld commented Nov 25, 2025

Uh oh!

azure-pipelines bot commented Nov 25, 2025

Uh oh!

Uh oh!

saiarcot895 Nov 26, 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 Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft commented Nov 25, 2025 •

edited

Loading

saiarcot895 Nov 25, 2025 •

edited by qiluo-msft

Loading

saiarcot895 Nov 25, 2025 •

edited by qiluo-msft

Loading

yue-fred-gao Nov 25, 2025 •

edited by qiluo-msft

Loading

yue-fred-gao Nov 25, 2025 •

edited by qiluo-msft

Loading

saiarcot895 Nov 26, 2025 •

edited by qiluo-msft

Loading

qiluo-msft Nov 26, 2025 •

edited

Loading

qiluo-msft Nov 26, 2025 •

edited

Loading