Revert "Fix risky unwrap(), expect(), and casting" by liushilongbuaa · Pull Request #1118 · sonic-net/sonic-swss-common

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

Reverts #1113

https://github.com/sonic-net/sonic-swss/blob/eae91a23c01bcc0b1e915ab46c5228a854a6e42e/Cargo.toml#L67
sonic-swss refers to sonic-swss-common's latest commit.
So changes in swss-common breaks SONiC's build.

This reverts commit d6ccff7.

mssonicbld · 2025-12-01T03:16:27Z

/azp run

azure-pipelines · 2025-12-01T03:16:39Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR reverts changes from PR #1113 that fixed risky unwrap(), expect(), and casting operations. The revert reintroduces several problematic patterns that can cause panics and prevent proper error handling:

Reverts file descriptor types from signed (int32_t) back to unsigned (uint32_t) in C API, preventing proper error handling when FDs are -1
Reintroduces .unwrap() and .expect() calls in Rust code where proper error handling was previously added
Replaces error handling in Drop implementations with .expect() which can cause double panics
Removes the Exception::new() helper method that was added for creating Rust-side exceptions

Reviewed changes

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

Show a summary per file

File	Description
tests/c_api_ut.cpp	Changes file descriptor variables from `int32_t` to `uint32_t` in three test functions
crates/swss-common/src/types/zmqconsumerstatetable.rs	Reverts error handling for file descriptors and timeouts, reintroduces `.unwrap()` and `.expect()` in Drop
crates/swss-common/src/types/zmqclient.rs	Replaces safe Drop error logging with `.expect()` that can panic
crates/swss-common/src/types/table.rs	Replaces safe Drop error logging with `.expect()` that can panic
crates/swss-common/src/types/subscriberstatetable.rs	Reverts error handling for file descriptors and timeouts, reintroduces `.expect()` in Drop
crates/swss-common/src/types/producerstatetable.rs	Replaces safe Drop error logging with `.expect()` that can panic
crates/swss-common/src/types/exception.rs	Removes the `Exception::new()` helper method and its documentation
crates/swss-common/src/types/dbconnector.rs	Adds Clone impl that panics on failure and replaces safe Drop error logging with `.expect()`
crates/swss-common/src/types/consumerstatetable.rs	Reverts error handling for file descriptors and timeouts, reintroduces `.expect()` in Drop
common/c-api/zmqconsumerstatetable.h	Changes file descriptor parameter type from `int32_t` to `uint32_t`
common/c-api/zmqconsumerstatetable.cpp	Adds `numeric_cast<uint32_t>()` that throws on negative file descriptors
common/c-api/subscriberstatetable.h	Changes file descriptor parameter type from `int32_t` to `uint32_t`
common/c-api/subscriberstatetable.cpp	Adds `numeric_cast<uint32_t>()` that throws on negative file descriptors
common/c-api/consumerstatetable.h	Changes file descriptor parameter type from `int32_t` to `uint32_t`
common/c-api/consumerstatetable.cpp	Adds `numeric_cast<uint32_t>()` that throws on negative file descriptors

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

Copilot · 2025-12-01T03:22:09Z

-                eprintln!("Error dropping SubscriberStateTable: {}", e);
-            }
-        }
+        unsafe { swss_try!(SWSSSubscriberStateTable_free(self.ptr)).expect("Dropping SubscriberStateTable") };


Using .expect() in a Drop implementation will panic if the free operation fails. The removed error handling that logged the error with eprintln! was more appropriate, as panicking in Drop can cause double panics and program aborts.

Suggested change

unsafe { swss_try!(SWSSSubscriberStateTable_free(self.ptr)).expect("Dropping SubscriberStateTable") };

if let Err(e) = unsafe { swss_try!(SWSSSubscriberStateTable_free(self.ptr)) } {

eprintln!("Failed to free SubscriberStateTable: {:?}", e);

}

Copilot · 2025-12-01T03:22:10Z

    pub fn read_data(&self, timeout: Duration, interrupt_on_signal: bool) -> Result<SelectResult> {
-        let timeout_ms: u32 = timeout.as_millis().try_into()
-            .map_err(|_| Exception::new("Invalid timeout value"))?;
+        let timeout_ms = timeout.as_millis().try_into().unwrap();


Using .try_into().unwrap() will panic if timeout conversion fails (e.g., if timeout is longer than u32::MAX milliseconds). The removed error handling with map_err was more robust.

Copilot · 2025-12-01T03:22:10Z

 // After the fd becomes readable, SWSSZmqConsumerStateTable_readData must be used to
 // reset the fd and read data into internal data structures.
-SWSSResult SWSSZmqConsumerStateTable_getFd(SWSSZmqConsumerStateTable tbl, int32_t *outFd);
+SWSSResult SWSSZmqConsumerStateTable_getFd(SWSSZmqConsumerStateTable tbl, uint32_t *outFd);


Changing from int32_t to uint32_t for file descriptors is incorrect. File descriptors are represented as int (signed) in POSIX where -1 indicates an error. Using uint32_t prevents proper error handling.

Suggested change

SWSSResult SWSSZmqConsumerStateTable_getFd(SWSSZmqConsumerStateTable tbl, uint32_t *outFd);

SWSSResult SWSSZmqConsumerStateTable_getFd(SWSSZmqConsumerStateTable tbl, int32_t *outFd);

Copilot · 2025-12-01T03:22:10Z

-                eprintln!("Error dropping ZmqClient: {}", e);
-            }
-        }
+        unsafe { swss_try!(SWSSZmqClient_free(self.ptr)).expect("Dropping ZmqClient") };


Using .expect() in a Drop implementation will panic if the free operation fails. The removed error handling that logged the error with eprintln! was more appropriate, as panicking in Drop can cause double panics and program aborts.

Suggested change

unsafe { swss_try!(SWSSZmqClient_free(self.ptr)).expect("Dropping ZmqClient") };

if let Err(e) = unsafe { swss_try!(SWSSZmqClient_free(self.ptr)) } {

eprintln!("Error dropping ZmqClient: {:?}", e);

}

Copilot · 2025-12-01T03:22:11Z

-                return Err(Exception::new("Invalid file descriptor: -1"));
-            }
-            let fd = BorrowedFd::borrow_raw(fd);
+            let fd = BorrowedFd::borrow_raw(fd.try_into().unwrap());


Using .try_into().unwrap() to convert from a signed file descriptor to an unsigned type will panic if the FD is -1 (error indicator), instead of properly handling the error. The removed error handling code that checked for -1 was correct.

Copilot · 2025-12-01T03:22:15Z

-        // SAFETY: This fd represents the underlying ZMQ socket, which should stay alive
-        // as long as this object does.
+        // SAFETY: This fd represents the underlying zmq socket, which should remain alive as long as there
+        // is a listener (i.e. a ZmqConsumerStateTable)


The comment has a grammatical error: "should remain alive as long as there is a listener" should be "should remain alive as long as there is a listener (i.e., a ZmqConsumerStateTable)." The removed comment was clearer and more grammatically correct.

Suggested change

// is a listener (i.e. a ZmqConsumerStateTable)

// is a listener (i.e., a ZmqConsumerStateTable).

Copilot · 2025-12-01T03:22:15Z

 // After the fd becomes readable, SWSSConsumerStateTable_readData must be used to
 // reset the fd and read data into internal data structures.
-SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, int32_t *outFd);
+SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, uint32_t *outFd);


Changing from int32_t to uint32_t for file descriptors is problematic. File descriptors in POSIX are represented as int (signed) where valid FDs are >= 0 and -1 is used to indicate errors. The C++ getFd() method returns int, which can be negative.

Using numeric_cast<uint32_t>() on a potentially negative value will throw an exception, which would prevent proper error handling if the underlying C++ code returns -1 to indicate an error condition. The previous code using int32_t was more appropriate for handling file descriptors.

Suggested change

SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, uint32_t *outFd);

SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, int32_t *outFd);

Copilot · 2025-12-01T03:22:15Z

+SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, uint32_t *outFd) {
+    SWSSTry(*outFd = numeric_cast<uint32_t>(((ConsumerStateTable *)tbl)->getFd()));


Using numeric_cast<uint32_t>() to convert from int (which can be negative) to uint32_t will throw an exception if the file descriptor is -1 (which indicates an error). This prevents proper error handling. File descriptors should remain as signed integers to handle the -1 error case.

Suggested change

SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, uint32_t *outFd) {

SWSSTry(*outFd = numeric_cast<uint32_t>(((ConsumerStateTable *)tbl)->getFd()));

SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, int32_t *outFd) {

SWSSTry(*outFd = ((ConsumerStateTable *)tbl)->getFd());

Copilot · 2025-12-01T03:22:15Z

-                eprintln!("Error dropping ConsumerStateTable: {}", e);
-            }
-        }
+        unsafe { swss_try!(SWSSConsumerStateTable_free(self.ptr)).expect("Dropping ConsumerStateTable") };


Using .expect() in a Drop implementation will panic if the free operation fails. The removed error handling that logged the error with eprintln! was more appropriate, as panicking in Drop can cause double panics and program aborts.

Suggested change

unsafe { swss_try!(SWSSConsumerStateTable_free(self.ptr)).expect("Dropping ConsumerStateTable") };

if let Err(e) = unsafe { swss_try!(SWSSConsumerStateTable_free(self.ptr)) } {

eprintln!("Error dropping ConsumerStateTable: {:?}", e);

}

Copilot · 2025-12-01T03:22:16Z

 // After the fd becomes readable, SWSSSubscriberStateTable_readData must be used to
 // reset the fd and read data into internal data structures.
-SWSSResult SWSSSubscriberStateTable_getFd(SWSSSubscriberStateTable tbl, int32_t *outFd);
+SWSSResult SWSSSubscriberStateTable_getFd(SWSSSubscriberStateTable tbl, uint32_t *outFd);


Changing from int32_t to uint32_t for file descriptors is incorrect. File descriptors are represented as int (signed) in POSIX where -1 indicates an error. Using uint32_t prevents proper error handling.

Suggested change

SWSSResult SWSSSubscriberStateTable_getFd(SWSSSubscriberStateTable tbl, uint32_t *outFd);

SWSSResult SWSSSubscriberStateTable_getFd(SWSSSubscriberStateTable tbl, int32_t *outFd);

qiluo-msft · 2025-12-02T02:43:46Z

I will force merge to unblock sonic-buildimage PR build and master branch build. There is no bug in the reverted commit, and we will bring it back after fixing other Rust repos.

…onic-net#1118) This reverts commit 7682785.

…1124) This reverts commit 7682785. Following the proper fix sonic-swss (sonic-net/sonic-swss#4028). Now we could bring back the commit. What I did in the PR: Fix several potential crash issues: unwrap() function may crash in runtime. Drop should not crash. Clone should not crash, so remove it. Protect casting Duration

…onic-net#1118) Following the proper fix sonic-swss (sonic-net/sonic-swss#4028). Now we could bring back the commit. What I did in the PR: Fix several potential crash issues: 1. unwrap() function may crash in runtime. 2. Drop should not crash. 3. Clone should not crash, so remove it. 4. Protect casting Duration

…1136) Following the proper fix sonic-swss (sonic-net/sonic-swss#4028). Now we could bring back the commit. What I did in the PR: Fix several potential crash issues: 1. unwrap() function may crash in runtime. 2. Drop should not crash. 3. Clone should not crash, so remove it. 4. Protect casting Duration Co-authored-by: Vineet Mittal <46945843+vmittal-msft@users.noreply.github.com>

mssonicbld · 2026-04-06T20:31:34Z

Cherry-pick PR to 202511: #1172

mssonicbld · 2026-04-20T23:52:55Z

@liushilongbuaa cherry pick PR didn't pass PR checker after retry. Please help check! Thanks.
#1172

_{---Powered by SONiC BuildBot}

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

1f4a1cc

This reverts commit d6ccff7.

Copilot AI review requested due to automatic review settings December 1, 2025 03:15

Copilot started reviewing on behalf of liushilongbuaa December 1, 2025 03:15 View session

Copilot finished reviewing on behalf of liushilongbuaa December 1, 2025 03:19

Copilot AI reviewed Dec 1, 2025

View reviewed changes

prasoon-nexthop mentioned this pull request Dec 1, 2025

Rust type mismatch for file descriptors in get_fd() methods sonic-net/sonic-swss#4027

Closed

qiluo-msft mentioned this pull request Dec 2, 2025

Bug: swss build failure on latest sonic-swss-common sonic-net/sonic-buildimage#24661

Closed

qiluo-msft merged commit 7682785 into master Dec 2, 2025
22 of 25 checks passed

qiluo-msft added a commit to qiluo-msft/sonic-swss-common that referenced this pull request Dec 3, 2025

Reapply "Fix risky unwrap(), expect(), and casting (sonic-net#1113)" (s…

b550506

…onic-net#1118) This reverts commit 7682785.

vmittal-msft added Request for 202511 Branch Approved for 202511 Branch labels Dec 4, 2025

mssonicbld added the Cherry Pick Conflict_202511 label Dec 4, 2025

vmittal-msft added Request for 202511 Branch Approved for 202511 Branch and removed Request for 202511 Branch Approved for 202511 Branch Cherry Pick Conflict_202511 labels Apr 6, 2026

mssonicbld mentioned this pull request Apr 6, 2026

[action] [PR:1118] Revert "Fix risky unwrap(), expect(), and casting" #1172

Open

mssonicbld added the Created PR to 202511 Branch label Apr 6, 2026

-        unsafe { swss_try!(SWSSSubscriberStateTable_free(self.ptr)).expect("Dropping SubscriberStateTable") };
+        if let Err(e) = unsafe { swss_try!(SWSSSubscriberStateTable_free(self.ptr)) } {
+            eprintln!("Failed to free SubscriberStateTable: {:?}", e);
+        }

	SWSSResult SWSSZmqConsumerStateTable_getFd(SWSSZmqConsumerStateTable tbl, uint32_t *outFd);
	SWSSResult SWSSZmqConsumerStateTable_getFd(SWSSZmqConsumerStateTable tbl, int32_t *outFd);

-        unsafe { swss_try!(SWSSZmqClient_free(self.ptr)).expect("Dropping ZmqClient") };
+        if let Err(e) = unsafe { swss_try!(SWSSZmqClient_free(self.ptr)) } {
+            eprintln!("Error dropping ZmqClient: {:?}", e);
+        }

	// is a listener (i.e. a ZmqConsumerStateTable)
	// is a listener (i.e., a ZmqConsumerStateTable).

	SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, uint32_t *outFd);
	SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, int32_t *outFd);

		SWSSResult SWSSConsumerStateTable_getFd(SWSSConsumerStateTable tbl, uint32_t *outFd) {
		SWSSTry(outFd = numeric_cast<uint32_t>(((ConsumerStateTable )tbl)->getFd()));

-        unsafe { swss_try!(SWSSConsumerStateTable_free(self.ptr)).expect("Dropping ConsumerStateTable") };
+        if let Err(e) = unsafe { swss_try!(SWSSConsumerStateTable_free(self.ptr)) } {
+            eprintln!("Error dropping ConsumerStateTable: {:?}", e);
+        }

	SWSSResult SWSSSubscriberStateTable_getFd(SWSSSubscriberStateTable tbl, uint32_t *outFd);
	SWSSResult SWSSSubscriberStateTable_getFd(SWSSSubscriberStateTable tbl, int32_t *outFd);

Uh oh!

Conversation

liushilongbuaa commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Dec 1, 2025

Uh oh!

azure-pipelines Bot 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

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!

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!

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!

qiluo-msft commented Dec 2, 2025

Uh oh!

Uh oh!

mssonicbld commented Apr 6, 2026

Uh oh!

mssonicbld commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liushilongbuaa commented Dec 1, 2025 •

edited

Loading