Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
6 changes: 3 additions & 3 deletions dnp3/src/app/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -371,14 +371,14 @@ pub(crate) struct HeaderCollection<'a> {

#[derive(Copy, Clone, Debug)]
pub(crate) enum SingleHeaderError {
BadParse,
BadParse(ObjectParseError),
NoHeaders,
MoreThanOneHeader,
}

impl From<ObjectParseError> for SingleHeaderError {
fn from(_: ObjectParseError) -> Self {
Self::BadParse
fn from(err: ObjectParseError) -> Self {
Self::BadParse(err)
}
}

Expand Down
9 changes: 7 additions & 2 deletions dnp3/src/master/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,8 +331,13 @@ impl From<ObjectParseError> for TaskError {
}

impl From<SingleHeaderError> 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
}
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions dnp3/src/master/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,8 @@ impl From<ObjectParseError> for FileError {
}

impl From<SingleHeaderError> for FileError {
fn from(_: SingleHeaderError) -> Self {
Self::TaskError(TaskError::UnexpectedResponseHeaders)
fn from(err: SingleHeaderError) -> Self {
Self::TaskError(err.into())
}
}

Expand Down
92 changes: 72 additions & 20 deletions dnp3/src/master/tests/file/read_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -108,6 +108,16 @@ fn open_response(seq: u8, status: FileStatus) -> Vec<u8> {
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<u8> {
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;
Expand Down Expand Up @@ -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
Expand All @@ -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(),
Expand Down Expand Up @@ -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<FileCredentials> {
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,
}
}

Expand All @@ -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))
}
}
}

Expand All @@ -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;
Expand All @@ -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(),
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -369,6 +408,7 @@ impl Ending {
| Self::BlockNumber
| Self::Handle
| Self::NoObjectHeader
| Self::Unparseable
| Self::ReaderAbort
| Self::ReaderAbortOnLastBlock => read_config(),
}
Expand All @@ -381,6 +421,7 @@ impl Ending {
| Self::BlockNumber
| Self::Handle
| Self::NoObjectHeader
| Self::Unparseable
| Self::MaxFileSize
| Self::MaxFileSizeOnLastBlock => false,
}
Expand All @@ -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)),
}
}

Expand All @@ -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")
}
Expand All @@ -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,
}
Expand All @@ -445,23 +489,31 @@ fn close_success(seq: u8) -> Vec<u8> {
}

/// 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<u8>, 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<u8>, 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),
]
}

Expand Down Expand Up @@ -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()) {
Expand Down
34 changes: 21 additions & 13 deletions dnp3/src/master/tests/outcomes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Loading
Loading