Repository navigation
Isobus and j1939acd fixes and improvements - #635
Merged
Merged
Conversation
Add expected_size field to test patterns to properly validate cases where the requested read size exceeds the actual file size. This allows testing the server's behavior when clients request more data than is available. When a read request is larger than the file, the server correctly returns data up to EOF. The test framework now distinguishes between this expected scenario and genuine read failures. Signed-off-by: Oleksij Rempel <linux@rempel-privat.de> [socketcan: addressed review comments from PR linux-can#629: - keep uint32_t instead of size_t for offset/read_size/expected_size: ISO 11783-13 limits the file offset (Seek File) and the file size (directory entry) to 32 bit, so uint32_t matches the protocol. The sum offset + read_size can exceed 32 bit (see UINT32_MAX pattern) and is therefore calculated in int64_t. size_t would not help here as it is only 32 bit wide on 32 bit targets. - remaining_size is a signed int64_t: the signed type is needed to detect the 'remaining_size < 0' case (server returned more data than requested) and int64_t holds the 33 bit sum on all platforms. - fix printf format for the uint32_t read_offset (%zu -> %u)] Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
The cmd_exit() function used -EINTR to signal a clean exit from the interactive mode. This is semantically incorrect because EINTR is an errno value meant for system call interruptions, not application-level control flow. Stop using -EINTR for cmd_exit(). Introduce ISOBUSFS_CLI_RET_EXIT (a positive return code) to distinguish control flow from actual errors. Map this to exit code 0 in main(), keeping negative errno values for real errors. This makes the exit path explicit and prevents confusion with actual interrupted system calls. Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
Fix directory operations to comply with ISO 11783-13:2021 (sections
C.3.4.2 and C.3.5.2), which specifies that directory operations differ
significantly from standard file operations:
- The Count parameter in a Read File request represents the number of
directory entries to read, not the number of bytes.
- The Offset parameter in a Seek File request represents the logical
entry index, not a byte offset.
The previous implementation treated directories strictly as files,
using byte-based offsets and counts. This resulted in incorrect seeking
behavior and protocol violations when listing directories.
Align the implementation with the standard by:
1. Server side:
- Introduce isobusfs_srv_dir_entry_visible() to consistently
filter out invalid (unreadable, hidden, oversized) entries. This
ensures that entry indices remain stable.
- Implement isobusfs_srv_dir_skip_entries() to advance the
directory stream by logical visible entries rather than bytes.
- Update the Read File handler to interpret count as the
maximum number of entries and return the number of entries read
in the response header.
- Update the Seek File handler to seek by entry index.
2. Client side:
- Calculate the request count based on the number of minimal-size
entries that fit into the maximum data length.
- Interpret the response count as the number of entries received
rather than the byte length of the payload.
Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
Read File and Seek File requests only checked the handle index to be within the handles array. Unused or closed handle slots are zeroed (fd = 0, dir = NULL), so a read request on such a handle read from the server's stdin and sent the content to the client. Additionally every client was able to read and seek handles opened by other clients. Add isobusfs_srv_get_client_handle() which only returns handles that are in use and have been opened by the requesting client. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
The Read File response buffer was allocated for the response header and the requested data only. Short responses (e.g. count = 0) are extended to ISOBUSFS_MIN_TRANSFER_LENGH which read beyond the allocated buffer and sent heap content to the client. Allocate at least ISOBUSFS_MIN_TRANSFER_LENGH bytes and fill the unused bytes with 0xff. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
The Change Current Directory and Volume Status requests used the path and volume name length from the request without checking it against the received message length. Short messages with a big name length made the server process stale content of the receive buffer as path or volume name. Check the name length against the message length, use le16toh() for the CCD name length and fix the debug output of the volume name. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
The Commanded Address message contains the 8 byte NAME and the new source address. The received length was not checked, so the new source address was taken from stale or uninitialized content of the receive buffer for short messages. Also the idle and global addresses were passed to claim_address() which terminates the daemon when bind() fails. Check for the complete message length and only accept unicast addresses. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
The path of an opened file or directory handle is allocated with strdup() but was not freed when the handle got closed. Every open/close sequence leaked the path string. Free the path before clearing the handle. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
The path of a handle is also used to mark the handle slot as used. When strdup() failed the handle was returned to the client but the slot was still marked as free and got reused (and the file descriptor of the first user got lost) with the next open request. Return -ENOMEM when the path can not be allocated. The callers close the file descriptor and the handle slot remains unused. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
…onse The switch statement which maps the errors of the current directory access check to the ISOBUS FS error codes had no break statements. Due to the fall through every error was reported as ISOBUSFS_ERR_OTHER. Add the missing break statements. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
The Change Current Directory and Open File requests assigned the path name length to the little endian name_len element without conversion, which leads to wrong values on big endian hosts. Use htole16() as it is already done in the other requests. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
The isobusfs protocol structures describe the message layout defined in ISO 11783-13 but were not packed. The compiler added padding bytes in front of 16/32 bit values which follow an odd number of bytes, e.g. the Read File Request (8 bytes according to C.3.5.2) had a size of 10 bytes and the count value at the wrong position. Server and client of the can-utils used the same structures and understood each other but the messages were not compatible to other ISOBUS devices. Additionally the padding bytes sent uninitialized content. Add __attribute__((__packed__)) to all protocol structures as it is already done in j1939_vehicle_position. Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Contributor
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The first three patches are from PR #629 that was stuck due to the review process. The review questions of commit #629 (review) have been addressed in this PR (see commit message)
The most important fix that has an impact on the data structures on the CAN bus is the last one ("isobusfs: pack the protocol structures").
"The isobusfs protocol structures describe the message layout defined in
ISO 11783-13 but were not packed. The compiler added padding bytes in
front of 16/32 bit values which follow an odd number of bytes, e.g. the
Read File Request (8 bytes according to C.3.5.2) had a size of 10 bytes
and the count value at the wrong position. Server and client of the
can-utils used the same structures and understood each other but the
messages were not compatible to other ISOBUS devices."
So this has to be checked and reviewed carefully as it breaks the former protocol of can-utils (but likely fits the rest of the world).