From 86a52bd2e2b664e6851f1fa664f7b6d94dd2cbcb Mon Sep 17 00:00:00 2001 From: jadamcrain Date: Thu, 22 Oct 2026 17:30:24 -0700 Subject: [PATCH 1/2] Report unparseable responses to tasks expecting no objects as MalformedResponse Empty-response, dead-band and time-sync WRITE tasks rejected any objects in the response as UnexpectedResponseHeaders without parsing them. Like the single-header tasks (#696), they now report objects that don't parse as MalformedResponse with the cause. Objects that parse are still unexpected. --- dnp3/src/master/tasks/deadbands.rs | 2 +- dnp3/src/master/tasks/empty_response.rs | 8 +-- dnp3/src/master/tasks/time.rs | 14 +--- dnp3/src/master/tests/outcomes.rs | 24 ++----- dnp3/src/master/tests/time_sync.rs | 86 +++++++++++++++++++++++++ 5 files changed, 99 insertions(+), 35 deletions(-) diff --git a/dnp3/src/master/tasks/deadbands.rs b/dnp3/src/master/tasks/deadbands.rs index 27279fab6..e8d223609 100644 --- a/dnp3/src/master/tasks/deadbands.rs +++ b/dnp3/src/master/tasks/deadbands.rs @@ -58,7 +58,7 @@ impl OneShotRequest for DeadBandsRequest { } fn parse(&self, response: Response<'_>) -> Result<(), WriteError> { - expect_empty(response) + Ok(expect_empty(&response)?) } fn wrap(task: OneShot) -> NonReadTask { diff --git a/dnp3/src/master/tasks/empty_response.rs b/dnp3/src/master/tasks/empty_response.rs index 51e0ac7f6..93e929798 100644 --- a/dnp3/src/master/tasks/empty_response.rs +++ b/dnp3/src/master/tasks/empty_response.rs @@ -17,11 +17,11 @@ impl EmptyResponseRequest { } /// Check that a response has no object headers -pub(crate) fn expect_empty(response: Response<'_>) -> Result<(), WriteError> { - if response.raw_objects.is_empty() { +pub(crate) fn expect_empty(response: &Response<'_>) -> Result<(), TaskError> { + if response.objects?.is_empty() { Ok(()) } else { - Err(WriteError::Task(TaskError::UnexpectedResponseHeaders)) + Err(TaskError::UnexpectedResponseHeaders) } } @@ -42,7 +42,7 @@ impl OneShotRequest for EmptyResponseRequest { } fn parse(&self, response: Response<'_>) -> Result<(), WriteError> { - expect_empty(response) + Ok(expect_empty(&response)?) } fn wrap(task: OneShot) -> NonReadTask { diff --git a/dnp3/src/master/tasks/time.rs b/dnp3/src/master/tasks/time.rs index 9564485c2..58646799d 100644 --- a/dnp3/src/master/tasks/time.rs +++ b/dnp3/src/master/tasks/time.rs @@ -10,6 +10,7 @@ use crate::master::association::Association; use crate::master::error::{TaskError, TimeSyncError}; use crate::master::promise::Promise; use crate::master::request::TimeSyncProcedure; +use crate::master::tasks::empty_response::expect_empty; use crate::master::tasks::{AppTask, NonReadTask, ResponseTask, Step, Task}; use crate::master::TaskType; @@ -119,18 +120,9 @@ impl From for Task { } } -/// Check that a response has no object headers -fn check_no_objects(response: &Response) -> Result<(), TimeSyncError> { - if response.raw_objects.is_empty() { - Ok(()) - } else { - Err(TimeSyncError::Task(TaskError::UnexpectedResponseHeaders)) - } -} - /// Check the response to the WRITE that sets the outstation's time fn check_write_response(response: &Response) -> Result<(), TimeSyncError> { - check_no_objects(response)?; + expect_empty(response)?; if response.header.iin.iin1.get_need_time() { return Err(TimeSyncError::StillNeedsTime); } @@ -257,7 +249,7 @@ impl ResponseTask for TimeSyncTask { Ok(Step::Next) } State::RecordCurrentTime(recorded_time) => { - check_no_objects(&response)?; + expect_empty(&response)?; self.state = State::WriteLastRecordedTime(recorded_time); Ok(Step::Next) } diff --git a/dnp3/src/master/tests/outcomes.rs b/dnp3/src/master/tests/outcomes.rs index 00bcca5e7..739582983 100644 --- a/dnp3/src/master/tests/outcomes.rs +++ b/dnp3/src/master/tests/outcomes.rs @@ -635,25 +635,11 @@ async fn no_task_confirms_a_response_with_con_whose_objects_do_not_parse() { // a valid fragment whose object header is cut short after its group let response = with_con(vec![0xC0, 0x81, 0x00, 0x00, 0x01]); respond(&mut started.harness, response).await; - let err = match kind { - Kind::Read - | Kind::DirectOperate - | Kind::SelectBeforeOperate - | Kind::ColdRestart - | Kind::AuthFile - | Kind::OpenFile - | Kind::CloseFile - | Kind::WriteFileBlock - | Kind::GetFileInfo - | Kind::ReadDirectory => { - TaskError::MalformedResponse(ObjectParseError::InsufficientBytes) - } - // these tasks expect no objects, and reject any without parsing them - Kind::EmptyResponse | Kind::DeadBands | Kind::TimeSync => { - TaskError::UnexpectedResponseHeaders - } - }; - started.expect_failure(err).await; + started + .expect_failure(TaskError::MalformedResponse( + ObjectParseError::InsufficientBytes, + )) + .await; assert_eq!(started.harness.io.pop_event(), None, "{kind:?}"); } }) diff --git a/dnp3/src/master/tests/time_sync.rs b/dnp3/src/master/tests/time_sync.rs index a43289bde..ec890a4ea 100644 --- a/dnp3/src/master/tests/time_sync.rs +++ b/dnp3/src/master/tests/time_sync.rs @@ -537,3 +537,89 @@ async fn user_non_lan_time_sync_fails_with_the_cause_when_the_delay_measure_resp }) .await; } + +/// A WRITE response with objects that parse is rejected as unexpected, and one with objects that +/// don't parse as malformed, with the cause +#[tokio::test(start_paused = true)] +async fn user_time_sync_fails_when_the_write_response_has_objects() { + watchdog(async { + let cases: [(&[u8], TaskError); 3] = [ + // a g52v2 time delay + ( + &[52, 2, 0x07, 1, 0x00, 0x00], + TaskError::UnexpectedResponseHeaders, + ), + // a header cut short after its group + ( + &[0x01], + TaskError::MalformedResponse(ObjectParseError::InsufficientBytes), + ), + // a variation of g50 that doesn't exist + ( + &[50, 9, 0x07, 1], + TaskError::MalformedResponse(ObjectParseError::UnknownGroupVariation(50, 9)), + ), + ]; + let procedures = [ + TimeSyncProcedure::Lan, + TimeSyncProcedure::NonLan, + TimeSyncProcedure::DirectWriteAbsTime, + ]; + for procedure in procedures { + for (objects, err) in cases { + let mut harness = + create_association_with_handler(config(None), Box::new(FixedTime)).await; + let mut seq = Sequence::default(); + + let mut association = harness.association.clone(); + let sync = + tokio::spawn(async move { association.synchronize_time(procedure).await }); + let (first, write) = match procedure { + TimeSyncProcedure::Lan => { + harness + .expect_write_and_respond( + record_current_time(seq), + empty_response(seq.increment()), + ) + .await; + ( + FunctionCode::RecordCurrentTime, + write_last_recorded_time(seq), + ) + } + TimeSyncProcedure::NonLan => { + harness + .expect_write_and_respond( + delay_measure(seq), + delay_measure_response(seq.increment(), 0), + ) + .await; + (FunctionCode::DelayMeasure, write_absolute_time(seq)) + } + TimeSyncProcedure::DirectWriteAbsTime => { + (FunctionCode::Write, write_absolute_time(seq)) + } + }; + let mut response = empty_response(seq); + response.extend_from_slice(objects); + harness.expect_write_and_respond(write, response).await; + + assert_eq!( + sync.await.unwrap(), + Err(TimeSyncError::Task(err)), + "{procedure:?} {objects:?}" + ); + assert_no_write_for(&mut harness, Duration::from_secs(60)).await; + assert_eq!( + harness.assoc_events.pop(), + [ + AssocInfoEvent::TaskStart(TaskType::TimeSync, first, Sequence::new(0)), + AssocInfoEvent::TaskFailure(TaskType::TimeSync, err), + ], + "{procedure:?} {objects:?}" + ); + } + } + }) + .await; +} From 162f52b38b046b0a171bfc29a8a8624818e6dc43 Mon Sep 17 00:00:00 2001 From: jadamcrain Date: Thu, 22 Oct 2026 17:32:06 -0700 Subject: [PATCH 2/2] Note the empty-response and deadband tasks in the MalformedResponse entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a53beb90..98e6f5f15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,7 +46,7 @@ * :bell: The master now confirms a response to a non-READ request (a control, restart, time sync, file or other task) when the outstation sets CON, before sending its next request, as IEEE 1815-2012 4.3 rule 13 requires. Responses whose objects can't be parsed are still not confirmed. See [#689](https://github.com/stepfunc/dnp3/pull/689). * :book: Add a guide page explaining the master's automatic tasks (startup, restart handling, event scans, time sync, keep-alive) and how a channel schedules them alongside user requests and polls. See [#700](https://github.com/stepfunc/dnp3/pull/700). * :bug: Fix an empty `AddressFilter::AnyOf` conflicting with `AddressFilter::Any` only when it was added second; an empty set now conflicts with no filter in either order. See [#697](https://github.com/stepfunc/dnp3/pull/697). -* :bell: Restart, time sync (delay measure) and file read tasks now fail with `MalformedResponse` and the parse cause when a response's objects can't be parsed, as commands and the other file tasks already do. In the bindings this can now be reported as an unsupported response instead of a bad one. `UnexpectedResponseHeaders` is still reported for well-formed responses with the wrong headers. See [#696](https://github.com/stepfunc/dnp3/pull/696). +* :bell: Restart, time sync, file read, `write_dead_bands` and `send_and_expect_empty_response` tasks now fail with `MalformedResponse` and the parse cause when a response's objects can't be parsed, as commands and the other file tasks already do. In the bindings this can now be reported as an unsupported response instead of a bad one. `UnexpectedResponseHeaders` is still reported for well-formed responses with the wrong headers. See [#696](https://github.com/stepfunc/dnp3/pull/696) and [#702](https://github.com/stepfunc/dnp3/pull/702). * :bell: An outstation that receives DISABLE_UNSOLICITED while waiting for an unsolicited confirm now keeps waiting, as TB2015-002a requires: a confirm still clears the reported events, and if the wait times out the series ends without a retry and the events are reported later. Before, a direct DISABLE_UNSOLICITED ended the series at once, so a late confirm couldn't clear the events and they were reported again, and a broadcast one left the response retrying. See [#691](https://github.com/stepfunc/dnp3/pull/691). * :bug: Fix the master reading 1-byte signed integer device attributes as unsigned, for example -1 as 255. See [#500](https://github.com/stepfunc/dnp3/pull/500). * :bug: Treat device attributes defined in `AttrSet::Private(0)` as the default set (0). They skipped the default set's type checks and a master couldn't read them. See [#565](https://github.com/stepfunc/dnp3/pull/565).