Report unparseable objects in empty-expected responses as MalformedResponse

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).

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<Self>) -> NonReadTask {

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<Self>) -> NonReadTask {

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<TimeSyncTask> 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)
             }

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:?}");
         }
     })

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;
+}