Amateurfunk
/
hamnet70
2
0
Fork 0
Code Issues Pull Requests 1 Projects Releases Wiki Activity

WIP: Layer 2-Implementierung #6

Draft
thomas wants to merge 8 commits from layer2_dev into main
pull from: layer2_dev
merge into: Amateurfunk:main
Conversation 0 Commits 8 Files Changed 6 +429
thomas commented 2024-07-01 21:00:31 +02:00
Owner
Copy Link

In diesem Branch wird das Layer2-Protokoll nach der Spezifikation in doc/hamnet70.adoc implementiert.

In diesem Branch wird das Layer2-Protokoll nach der Spezifikation in `doc/hamnet70.adoc` implementiert.
thomas added 4 commits 2024-07-01 21:00:31 +02:00
thomas self-assigned this 2024-07-01 21:00:40 +02:00
thomas changed title from Layer 2-Implementierung to WIP: Layer 2-Implementierung 2024-07-01 21:00:49 +02:00
thomas force-pushed layer2_dev from 0fc63f5f69 to 4bb4017623 2024-07-01 21:04:18 +02:00 Compare
thomas added 1 commit 2024-07-01 21:26:14 +02:00
rudi_s requested changes 2024-07-03 09:01:51 +02:00
Dismissed
rudi_s left a comment
Collaborator
Copy Link
No description provided.
test_ham64.c.txt
1.5 KiB
impl/src/layer2/ham64.c Outdated
@ -0,0 +51,4 @@
uint16_t *tmp = ham64->addr;
memset(ham64->addr, 0, 4*sizeof(uint16_t));
rudi_s commented 2024-07-03 08:11:47 +02:00
Collaborator
Copy Link

memset(ham64->addr, 0, sizeof(ham64->addr));

`memset(ham64->addr, 0, sizeof(ham64->addr));`
thomas commented 2024-07-04 20:58:38 +02:00
Author
Owner
Copy Link

Da war ich mir nicht sicher, ob das sizeof(ham64->addr) das richtige tut oder doch die Größe von einem Pointer liefert. Aber müsste schon passen. Danke.

Da war ich mir nicht sicher, ob das `sizeof(ham64->addr)` das richtige tut oder doch die Größe von einem Pointer liefert. Aber müsste schon passen. Danke.
rudi_s commented 2024-07-04 23:20:44 +02:00
Collaborator
Copy Link

sizeof liefert die Größe des jeweiligen Objekts, das funktioniert soweit ich weiß immer. Ausnahmen sind nur so Dinge wie foo[] am Ende vom struct (da ist dann sizeof == 0).

`sizeof` liefert die Größe des jeweiligen Objekts, das funktioniert soweit ich weiß immer. Ausnahmen sind nur so Dinge wie `foo[]` am Ende vom struct (da ist dann `sizeof == 0`).
rudi_s marked this conversation as resolved
impl/src/layer2/ham64.c Outdated
@ -0,0 +137,4 @@
"BROADCAST",
"IPV6_MULTICAST",
"IPV4_MULTICAST",
"RESERVED"};
rudi_s commented 2024-07-03 08:20:13 +02:00
Collaborator
Copy Link

Minor nit: Put }; on a new line and add a trailing , after "RESERVED"?

Minor nit: Put `};` on a new line and add a trailing `,` after `"RESERVED"`?
rudi_s marked this conversation as resolved
impl/src/layer2/ham64.c
@ -0,0 +173,4 @@
void ham64_format(const ham64_t *ham64, char *out)
{
for(uint8_t i = 0; i < ham64->length; i++) {
sprintf(out + 5 * i, "%04X-", ham64->addr[i]);
rudi_s commented 2024-07-03 08:57:33 +02:00
Collaborator
Copy Link

Off-by-one overflow (found by fsanitize) if ham64->length == 4 because sprintf NUL-terminates its output but there's not enough space for - and NUL. Possible fix:

 void ham64_format(const ham64_t *ham64, char *out)
 {
        for(uint8_t i = 0; i < ham64->length; i++) {
-               sprintf(out + 5 * i, "%04X-", ham64->addr[i]);
+               const char *fmt = i == 3 ? "%04X" : "%04X-";
+               sprintf(out + 5 * i, fmt, ham64->addr[i]);
        }

        out[5*ham64->length - 1] = '\0';
Off-by-one overflow (found by fsanitize) if `ham64->length == 4` because `sprintf` NUL-terminates its output but there's not enough space for `-` and NUL. Possible fix: ``` void ham64_format(const ham64_t *ham64, char *out) { for(uint8_t i = 0; i < ham64->length; i++) { - sprintf(out + 5 * i, "%04X-", ham64->addr[i]); + const char *fmt = i == 3 ? "%04X" : "%04X-"; + sprintf(out + 5 * i, fmt, ham64->addr[i]); } out[5*ham64->length - 1] = '\0'; ```
rudi_s marked this conversation as resolved
impl/src/layer2/layer2_structs.c Outdated
@ -0,0 +45,4 @@
memcpy(header->src_addr.addr, encoded + 2, 2 * header->src_addr.length);
memcpy(header->dst_addr.addr, encoded + 2 + 2 * header->src_addr.length, 2 * header->dst_addr.length);
rudi_s commented 2024-07-03 08:13:54 +02:00
Collaborator
Copy Link

I think the length of encoded should be passed as well or invalid src/dst lengths can cause an overread.

I think the length of `encoded` should be passed as well or invalid src/dst lengths can cause an overread.
rudi_s marked this conversation as resolved
impl/src/layer2/layer2_structs.h Outdated
@ -0,0 +21,4 @@
uint8_t tx_seq_nr; //!< sequence number of this packet
uint8_t rx_seq_nr; //!< sequence number expected next from he other side
rudi_s commented 2024-07-03 08:15:47 +02:00
Collaborator
Copy Link

Add the maximum value (15) to the comment as the whole uint_8 cannot be used?

Add the maximum value (15) to the comment as the whole `uint_8` cannot be used?
rudi_s marked this conversation as resolved
impl/test/test_ham64.c Outdated
@ -0,0 +1,50 @@
#include <stdio.h>
rudi_s commented 2024-07-03 08:53:55 +02:00
Collaborator
Copy Link

I attached test_ham64.c as suggestion to fix the issues I described below.

I attached `test_ham64.c` as suggestion to fix the issues I described below.
rudi_s marked this conversation as resolved
impl/test/test_ham64.c Outdated
@ -0,0 +7,4 @@
bool test_decode(const char *ref, const ham64_t *ham64)
{
char decoded[13];
decoded[12] = 0;
rudi_s commented 2024-07-03 08:55:28 +02:00
Collaborator
Copy Link

Only initializing the last byte is problematic because ham64_decode_callsign doesn't NUL-terminate with unsupported inputs, causing unitialized stack data to be printed (and with my patch validated) below.

Only initializing the last byte is problematic because `ham64_decode_callsign` doesn't NUL-terminate with unsupported inputs, causing unitialized stack data to be printed (and with my patch validated) below.
rudi_s marked this conversation as resolved
impl/test/test_ham64.c
@ -0,0 +15,4 @@
ham64_format(ham64, ham64_format_buf);
const char *typestr = ham64_addr_type_to_string(ham64_get_addr_type(ham64));
printf("»%s« → [%u] %s (%s) → [%zd] »%s«\n", ref, ham64->length, ham64_format_buf, typestr, decoded_len, decoded);
rudi_s commented 2024-07-03 08:26:51 +02:00
Collaborator
Copy Link

These printed results should be verified by the test as well.

These printed results should be verified by the test as well.
rudi_s marked this conversation as resolved
impl/test/test_ham64.c Outdated
@ -0,0 +47,4 @@
test_decode("", &short_1);
test_decode("", &short_last);
test_decode("", &broadcast);
}
rudi_s commented 2024-07-03 08:27:08 +02:00
Collaborator
Copy Link

The return values are not checked.

The return values are not checked.
rudi_s marked this conversation as resolved
thomas added 2 commits 2024-07-04 20:54:29 +02:00
thomas added 1 commit 2024-07-04 21:12:25 +02:00
thomas force-pushed layer2_dev from ee5c62167a to 153ff61dcd 2024-07-04 21:19:32 +02:00 Compare
thomas added 1 commit 2024-07-04 21:37:46 +02:00
rudi_s approved these changes 2024-07-04 23:21:05 +02:00
rudi_s left a comment
Collaborator
Copy Link

Looks good now :-)

Looks good now :-)
This pull request is marked as a work in progress.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b layer2_dev main
git pull origin layer2_dev

Step 2:

Merge the changes and update on Forgejo.
git checkout main
git merge --no-ff layer2_dev
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
rudi_s
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
thomas
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Amateurfunk/hamnet70#6
No description provided.
Delete Branch "layer2_dev"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?