From 61a5dd9d39452b0c9382308b581774f76f97a125 Mon Sep 17 00:00:00 2001 From: jadamcrain Date: Thu, 22 Oct 2026 16:04:32 -0700 Subject: [PATCH 1/2] Report unparseable single-header responses as MalformedResponse Restart, delay measure and file read read their response through get_only_object_header, whose error conversions turned a parse failure into UnexpectedResponseHeaders and dropped the ObjectParseError. Commands and the other file tasks already report MalformedResponse with the cause, so carry the cause in SingleHeaderError::BadParse and map it the same way. UnexpectedResponseHeaders remains for well-formed responses with no header, more than one, or the wrong one. --- dnp3/src/app/parse/parser.rs | 6 +- dnp3/src/master/error.rs | 9 ++- dnp3/src/master/file.rs | 4 +- dnp3/src/master/tests/file/read_file.rs | 92 +++++++++++++++++++------ dnp3/src/master/tests/outcomes.rs | 34 +++++---- dnp3/src/master/tests/restart.rs | 36 +++++++++- dnp3/src/master/tests/time_sync.rs | 54 ++++++++++++++- 7 files changed, 192 insertions(+), 43 deletions(-) diff --git a/dnp3/src/app/parse/parser.rs b/dnp3/src/app/parse/parser.rs index 25b095d31..d1b4d45a8 100644 --- a/dnp3/src/app/parse/parser.rs +++ b/dnp3/src/app/parse/parser.rs @@ -371,14 +371,14 @@ pub(crate) struct HeaderCollection<'a> { #[derive(Copy, Clone, Debug)] pub(crate) enum SingleHeaderError { - BadParse, + BadParse(ObjectParseError), NoHeaders, MoreThanOneHeader, } impl From for SingleHeaderError { - fn from(_: ObjectParseError) -> Self { - Self::BadParse + fn from(err: ObjectParseError) -> Self { + Self::BadParse(err) } } diff --git a/dnp3/src/master/error.rs b/dnp3/src/master/error.rs index 62bd43914..47bbb3a38 100644 --- a/dnp3/src/master/error.rs +++ b/dnp3/src/master/error.rs @@ -331,8 +331,13 @@ impl From for TaskError { } impl From for TaskError { - fn from(_: SingleHeaderError) -> Self { - TaskError::UnexpectedResponseHeaders + fn from(err: SingleHeaderError) -> Self { + match err { + SingleHeaderError::BadParse(err) => TaskError::MalformedResponse(err), + SingleHeaderError::NoHeaders | SingleHeaderError::MoreThanOneHeader => { + TaskError::UnexpectedResponseHeaders + } + } } } diff --git a/dnp3/src/master/file.rs b/dnp3/src/master/file.rs index 2851eb8bb..9f647b532 100644 --- a/dnp3/src/master/file.rs +++ b/dnp3/src/master/file.rs @@ -166,8 +166,8 @@ impl From for FileError { } impl From for FileError { - fn from(_: SingleHeaderError) -> Self { - Self::TaskError(TaskError::UnexpectedResponseHeaders) + fn from(err: SingleHeaderError) -> Self { + Self::TaskError(err.into()) } } diff --git a/dnp3/src/master/tests/file/read_file.rs b/dnp3/src/master/tests/file/read_file.rs index a261fbbe9..59c5f7acd 100644 --- a/dnp3/src/master/tests/file/read_file.rs +++ b/dnp3/src/master/tests/file/read_file.rs @@ -3,8 +3,8 @@ use std::time::Duration; use tokio::task::JoinHandle; use crate::app::{ - FileStatus, FunctionCode, Group70Var3, Group70Var5, Iin, Iin1, Iin2, Permissions, Sequence, - Shutdown, Timestamp, + FileStatus, FunctionCode, Group70Var3, Group70Var5, Iin, Iin1, Iin2, ObjectParseError, + Permissions, Sequence, Shutdown, Timestamp, }; use crate::master::association::AssociationConfig; use crate::master::tasks::file::REQUEST_ID; @@ -108,6 +108,16 @@ fn open_response(seq: u8, status: FileStatus) -> Vec { super::file_status_response(seq, HANDLE, 8, 128, status) } +/// Why [`unparseable_response`] doesn't parse +const UNPARSEABLE: ObjectParseError = ObjectParseError::UnknownGroupVariation(70, 99); + +/// A response whose object header is a variation of g70 that doesn't exist +fn unparseable_response(seq: u8) -> Vec { + let mut response = super::no_object_response(seq); + response.extend_from_slice(&[70, 99]); + response +} + /// Open without credentials and expect the READ of block 0 async fn open(harness: &mut TestHarness) { harness.expect_write(open_request(0, 0)).await; @@ -151,8 +161,8 @@ async fn read_to_close(harness: &mut TestHarness, reader: &RecordingReader) { enum Finished { /// `task_success`, with the sequence number of the last request Success(u8), - /// `task_fail` with `UnexpectedResponseHeaders` - Failure, + /// `task_fail` with this error + Failure(TaskError), } /// Check that the read task, whose first request had function code `first`, finished this way and @@ -163,9 +173,7 @@ async fn expect_finished(harness: &mut TestHarness, first: FunctionCode, finishe Finished::Success(seq) => { AssocInfoEvent::TaskSuccess(TaskType::FileRead, first, Sequence::new(seq)) } - Finished::Failure => { - AssocInfoEvent::TaskFailure(TaskType::FileRead, TaskError::UnexpectedResponseHeaders) - } + Finished::Failure(err) => AssocInfoEvent::TaskFailure(TaskType::FileRead, err), }; assert_eq!( harness.assoc_events.pop(), @@ -252,21 +260,30 @@ enum OpenFailure { OpenVariation, /// OPEN is answered without an object header NoObjectHeader, + /// AUTHENTICATE is answered with an object header that doesn't parse + AuthUnparseable, + /// OPEN is answered with an object header that doesn't parse + OpenUnparseable, } -const OPEN_FAILURES: [OpenFailure; 5] = [ +const OPEN_FAILURES: [OpenFailure; 7] = [ OpenFailure::NoPermission, OpenFailure::AuthVariation, OpenFailure::OpenStatus, OpenFailure::OpenVariation, OpenFailure::NoObjectHeader, + OpenFailure::AuthUnparseable, + OpenFailure::OpenUnparseable, ]; impl OpenFailure { fn credentials(self) -> Option { match self { - Self::NoPermission | Self::AuthVariation => Some(credentials()), - Self::OpenStatus | Self::OpenVariation | Self::NoObjectHeader => None, + Self::NoPermission | Self::AuthVariation | Self::AuthUnparseable => Some(credentials()), + Self::OpenStatus + | Self::OpenVariation + | Self::NoObjectHeader + | Self::OpenUnparseable => None, } } @@ -284,6 +301,23 @@ impl OpenFailure { Self::AuthVariation | Self::OpenVariation => FileError::BadResponse, Self::OpenStatus => FileError::BadStatus(FileStatus::FileNotFound), Self::NoObjectHeader => FileError::TaskError(TaskError::UnexpectedResponseHeaders), + Self::AuthUnparseable | Self::OpenUnparseable => { + FileError::TaskError(TaskError::MalformedResponse(UNPARSEABLE)) + } + } + } + + /// How the `AssociationInformation` sees the task end + fn finished(self) -> Finished { + match self { + Self::NoPermission + | Self::AuthVariation + | Self::OpenStatus + | Self::OpenVariation + | Self::NoObjectHeader => Finished::Failure(TaskError::UnexpectedResponseHeaders), + Self::AuthUnparseable | Self::OpenUnparseable => { + Finished::Failure(TaskError::MalformedResponse(UNPARSEABLE)) + } } } @@ -298,6 +332,8 @@ impl OpenFailure { ), Self::OpenVariation => (open_request(0, 0), auth_response(0, 7)), Self::NoObjectHeader => (open_request(0, 0), super::no_object_response(0)), + Self::AuthUnparseable => (AUTH_REQUEST.to_vec(), unparseable_response(0)), + Self::OpenUnparseable => (open_request(0, 0), unparseable_response(0)), }; harness.expect_write(request).await; harness.process_response(response).await; @@ -311,7 +347,7 @@ async fn open_failures_abort_the_reader_and_fail_the_task_without_a_close() { let mut harness = create_association(AssociationConfig::quiet()).await; let reader = start_read(&mut harness, read_config(), failure.credentials()).await; failure.answer(&mut harness).await; - expect_finished(&mut harness, failure.first(), Finished::Failure).await; + expect_finished(&mut harness, failure.first(), failure.finished()).await; harness.assert_no_events(); assert_eq!( reader.pop(), @@ -334,6 +370,8 @@ enum Ending { Handle, /// READ is answered without an object header NoObjectHeader, + /// READ is answered with an object header that doesn't parse + Unparseable, /// a block that isn't the last exceeds the configured maximum file size MaxFileSize, /// the last block exceeds the configured maximum file size @@ -344,11 +382,12 @@ enum Ending { ReaderAbortOnLastBlock, } -const ENDINGS: [Ending; 8] = [ +const ENDINGS: [Ending; 9] = [ Ending::BlockVariation, Ending::BlockNumber, Ending::Handle, Ending::NoObjectHeader, + Ending::Unparseable, Ending::MaxFileSize, Ending::MaxFileSizeOnLastBlock, Ending::ReaderAbort, @@ -369,6 +408,7 @@ impl Ending { | Self::BlockNumber | Self::Handle | Self::NoObjectHeader + | Self::Unparseable | Self::ReaderAbort | Self::ReaderAbortOnLastBlock => read_config(), } @@ -381,6 +421,7 @@ impl Ending { | Self::BlockNumber | Self::Handle | Self::NoObjectHeader + | Self::Unparseable | Self::MaxFileSize | Self::MaxFileSizeOnLastBlock => false, } @@ -395,8 +436,9 @@ impl Ending { | Self::ReaderAbort | Self::ReaderAbortOnLastBlock => closed, Self::BlockVariation | Self::BlockNumber | Self::Handle | Self::NoObjectHeader => { - Finished::Failure + Finished::Failure(TaskError::UnexpectedResponseHeaders) } + Self::Unparseable => Finished::Failure(TaskError::MalformedResponse(UNPARSEABLE)), } } @@ -408,6 +450,7 @@ impl Ending { super::file_transport_response(1, HANDLE + 1, super::last_block(0), b"data") } Self::NoObjectHeader => super::no_object_response(1), + Self::Unparseable => unparseable_response(1), Self::MaxFileSize | Self::ReaderAbort => { super::file_transport_response(1, HANDLE, 0, b"data") } @@ -424,6 +467,7 @@ impl Ending { Self::BlockNumber => FileError::BadBlockNum, Self::Handle => FileError::WrongHandle, Self::NoObjectHeader => FileError::TaskError(TaskError::UnexpectedResponseHeaders), + Self::Unparseable => FileError::TaskError(TaskError::MalformedResponse(UNPARSEABLE)), Self::MaxFileSize | Self::MaxFileSizeOnLastBlock => FileError::MaxLengthExceeded, Self::ReaderAbort | Self::ReaderAbortOnLastBlock => FileError::AbortByUser, } @@ -445,23 +489,31 @@ fn close_success(seq: u8) -> Vec { } /// Every CLOSE response with how it ends a task that it decides: success, and the responses that -/// fail validation (a bad status, the wrong handle, the wrong variation, and no object header) -fn close_responses(seq: u8) -> [(Vec, Finished); 5] { +/// fail validation (a bad status, the wrong handle, the wrong variation, no object header, and an +/// object header that doesn't parse) +fn close_responses(seq: u8) -> [(Vec, Finished); 6] { [ (close_success(seq), Finished::Success(seq)), ( super::file_status_response(seq, HANDLE, 0, 0, FileStatus::InvalidHandle), - Finished::Failure, + Finished::Failure(TaskError::UnexpectedResponseHeaders), ), ( super::file_status_response(seq, HANDLE + 1, 0, 0, FileStatus::Success), - Finished::Failure, + Finished::Failure(TaskError::UnexpectedResponseHeaders), ), ( super::file_transport_response(seq, HANDLE, 0, b""), - Finished::Failure, + Finished::Failure(TaskError::UnexpectedResponseHeaders), + ), + ( + super::no_object_response(seq), + Finished::Failure(TaskError::UnexpectedResponseHeaders), + ), + ( + unparseable_response(seq), + Finished::Failure(TaskError::MalformedResponse(UNPARSEABLE)), ), - (super::no_object_response(seq), Finished::Failure), ] } @@ -714,7 +766,7 @@ async fn directory_read_fails_with_the_error_that_ended_the_transfer() { let reply = start_directory_read(&harness, read_config(), failure.credentials()); failure.answer(&mut harness).await; assert_eq!(reply.await.unwrap(), Err(failure.error()), "{failure:?}"); - expect_finished(&mut harness, failure.first(), Finished::Failure).await; + expect_finished(&mut harness, failure.first(), failure.finished()).await; harness.assert_no_events(); } for ending in ENDINGS.into_iter().filter(|ending| !ending.reader_aborts()) { diff --git a/dnp3/src/master/tests/outcomes.rs b/dnp3/src/master/tests/outcomes.rs index 038cec86f..00bcca5e7 100644 --- a/dnp3/src/master/tests/outcomes.rs +++ b/dnp3/src/master/tests/outcomes.rs @@ -12,7 +12,8 @@ use tokio::time::Instant; use crate::app::control::{Group12Var1, OpType}; use crate::app::{ ControlField, FileStatus, FileType, FunctionCode, Group70Var2, Group70Var6, Group70Var7, Iin, - Iin1, Iin2, MaybeAsync, Permissions, ResponseHeader, Sequence, Timestamp, Variation, + Iin1, Iin2, MaybeAsync, ObjectParseError, Permissions, ResponseHeader, Sequence, Timestamp, + Variation, }; use crate::link::{EndpointAddress, LinkError}; use crate::master::association::AssociationConfig; @@ -634,19 +635,26 @@ 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; - assert_ne!(started.reply().await, kind.ok(), "{kind:?}"); - barrier(&mut started.harness).await; - // the task, not the transport, rejects it - assert!( - matches!( - started.harness.assoc_events.pop().as_slice(), - [AssocInfoEvent::TaskFailure(task_type, err)] - if *task_type == kind.task_type() && *err != TaskError::Transport - ), - "{kind:?}" - ); + 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; assert_eq!(started.harness.io.pop_event(), None, "{kind:?}"); - assert!(!started.harness.task.is_finished(), "{kind:?}"); } }) .await; diff --git a/dnp3/src/master/tests/restart.rs b/dnp3/src/master/tests/restart.rs index ffae7ebed..b32a1c724 100644 --- a/dnp3/src/master/tests/restart.rs +++ b/dnp3/src/master/tests/restart.rs @@ -1,6 +1,6 @@ use std::time::Duration; -use crate::app::Sequence; +use crate::app::{ObjectParseError, Sequence}; use crate::master::association::AssociationConfig; use crate::master::TaskError; @@ -35,7 +35,7 @@ struct Case { expected: Result, } -const CASES: [Case; 10] = [ +const CASES: [Case; 14] = [ // g52v1 with a count of one: 2 seconds Case { restart: Restart::Cold, @@ -96,6 +96,34 @@ const CASES: [Case; 10] = [ objects: &[52, 2, 0x07, 1, 0x02, 0x00, 52, 2, 0x07, 1, 0x03, 0x00], expected: Err(TaskError::UnexpectedResponseHeaders), }, + // no object header + Case { + restart: Restart::Cold, + objects: &[], + expected: Err(TaskError::UnexpectedResponseHeaders), + }, + // two g52v1 time delays + Case { + restart: Restart::Cold, + objects: &[52, 1, 0x07, 1, 0x02, 0x00, 52, 1, 0x07, 1, 0x03, 0x00], + expected: Err(TaskError::UnexpectedResponseHeaders), + }, + // g52v1 cut short after its qualifier + Case { + restart: Restart::Cold, + objects: &[52, 1, 0x07], + expected: Err(TaskError::MalformedResponse( + ObjectParseError::InsufficientBytes, + )), + }, + // a variation of g52 that doesn't exist + Case { + restart: Restart::Warm, + objects: &[52, 9, 0x07, 1, 0x02, 0x00], + expected: Err(TaskError::MalformedResponse( + ObjectParseError::UnknownGroupVariation(52, 9), + )), + }, ]; #[tokio::test] @@ -126,6 +154,10 @@ async fn restart_returns_the_time_delay_of_the_response() { case.restart, case.objects ); + + // nothing else is sent once the master has processed the response + harness.master.get_decode_level().await.unwrap(); + harness.assert_no_events(); } }) .await; diff --git a/dnp3/src/master/tests/time_sync.rs b/dnp3/src/master/tests/time_sync.rs index 04061d950..a43289bde 100644 --- a/dnp3/src/master/tests/time_sync.rs +++ b/dnp3/src/master/tests/time_sync.rs @@ -8,7 +8,9 @@ use tokio::time::Instant; use crate::app::format::write::{start_request, start_response}; use crate::app::variations::{Group50Var1, Group50Var3, Group52Var2}; use crate::app::Timestamp; -use crate::app::{ControlField, FunctionCode, Iin, Iin1, Iin2, ResponseFunction, Sequence}; +use crate::app::{ + ControlField, FunctionCode, Iin, Iin1, Iin2, ObjectParseError, ResponseFunction, Sequence, +}; use crate::master::association::AssociationConfig; use crate::master::tests::harness::AssocInfoEvent; use crate::master::{AssociationHandler, TaskError, TaskType, TimeSyncError, TimeSyncProcedure}; @@ -485,3 +487,53 @@ async fn user_time_sync_that_still_needs_time_fails_and_starts_the_auto_time_syn }) .await; } + +#[tokio::test(start_paused = true)] +async fn user_non_lan_time_sync_fails_with_the_cause_when_the_delay_measure_response_does_not_parse( +) { + watchdog(async { + let cases: [(&[u8], ObjectParseError); 2] = [ + // g52v2 cut short after its qualifier + (&[52, 2, 0x07], ObjectParseError::InsufficientBytes), + // a variation of g52 that doesn't exist + ( + &[52, 9, 0x07, 1, 0x00, 0x00], + ObjectParseError::UnknownGroupVariation(52, 9), + ), + ]; + for (objects, cause) in cases { + let mut harness = + create_association_with_handler(config(None), Box::new(FixedTime)).await; + let seq = Sequence::default(); + + let mut association = harness.association.clone(); + let sync = tokio::spawn(async move { + association + .synchronize_time(TimeSyncProcedure::NonLan) + .await + }); + let mut response = empty_response(seq); + response.extend_from_slice(objects); + harness + .expect_write_and_respond(delay_measure(seq), response) + .await; + + let err = TaskError::MalformedResponse(cause); + assert_eq!( + sync.await.unwrap(), + Err(TimeSyncError::Task(err)), + "{objects:?}" + ); + assert_no_write_for(&mut harness, Duration::from_secs(60)).await; + assert_eq!( + harness.assoc_events.pop(), + [ + AssocInfoEvent::TaskStart(TaskType::TimeSync, FunctionCode::DelayMeasure, seq), + AssocInfoEvent::TaskFailure(TaskType::TimeSync, err), + ], + "{objects:?}" + ); + } + }) + .await; +} From b12deea2b7dde0575c4d04c37bc8528787f87579 Mon Sep 17 00:00:00 2001 From: jadamcrain Date: Thu, 22 Oct 2026 16:06:12 -0700 Subject: [PATCH 2/2] Add a changelog entry for #696 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f32bb0d0..2eb8e4079 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ * :bell: Log expected traffic at debug instead of warn (frames and responses from other outstations, repeated segments, segments that arrive while a fragment waits), and log unsupported or ignored object headers once per fragment or request, with a count. See [#685](https://github.com/stepfunc/dnp3/pull/685). * :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). * :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: 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).