Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Address UDP connect message endian issue #1415

Closed
wants to merge 1 commit into from
Closed

Address UDP connect message endian issue #1415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions src/iperf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,9 +430,10 @@ struct iperf_test
extern int gerror; /* error value from getaddrinfo(3), for use in internal error handling */

/* UDP "connect" message and reply (textual value for Wireshark, etc. readability - legacy was numeric) */
#define UDP_CONNECT_MSG 0x36373839 // "6789" - legacy value was 123456789
#define UDP_CONNECT_REPLY 0x39383736 // "9876" - legacy value was 987654321
#define LEGACY_UDP_CONNECT_REPLY 987654321 // Old servers may still reply with the legacy value
#define UDP_CONNECT_MSG 0x36373839 // "6789" - legacy value was 123456789
#define UDP_CONNECT_REPLY 0x39383736 // "9876" - legacy value was 987654321
#define LEGACY_UDP_CONNECT_REPLY 987654321 // Old servers may still reply with legacy value
#define LEGACY_UDP_CONNECT_REPLY_ENDIAN 2976439866 // Old servers may still reply with legacy value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per issue #1608, reverse Endian of UDP_CONNECT_REPLY should also be considered. therefore should also add:

#define UDP_CONNECT_REPLY_ENDIAN 0x36373839


/* In Reverse mode, maximum number of packets to wait for "accept" response - to handle out of order packets */
#define MAX_REVERSE_OUT_OF_ORDER_PACKETS 2
Expand Down
4 changes: 2 additions & 2 deletions src/iperf_udp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,9 +592,9 @@ iperf_udp_connect(struct iperf_test *test)
printf("Connect received for Socket %d, sz=%d, buf=%x, i=%d, max_len_wait_for_reply=%d\n", s, sz, buf, i, max_len_wait_for_reply);
}
i += sz;
} while (buf != UDP_CONNECT_REPLY && buf != LEGACY_UDP_CONNECT_REPLY && i < max_len_wait_for_reply);
} while (buf != UDP_CONNECT_REPLY && buf != LEGACY_UDP_CONNECT_REPLY && buf != UDP_CONNECT_MSG && buf != LEGACY_UDP_CONNECT_REPLY_ENDIAN && i < max_len_wait_for_reply);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check also for UDP_CONNECT_REPLY_ENDIAN.


if (buf != UDP_CONNECT_REPLY && buf != LEGACY_UDP_CONNECT_REPLY) {
if (buf != UDP_CONNECT_REPLY && buf != LEGACY_UDP_CONNECT_REPLY && buf != UDP_CONNECT_MSG && buf != LEGACY_UDP_CONNECT_REPLY_ENDIAN) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check also for UDP_CONNECT_REPLY_ENDIAN.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already covered by my pull request. but the iperf3 maintainer seem to ignore all pull requests and bug fixes since a long time. i fixed it in my own tree and i'm happy with it. every effort here is pointless. check my pull request. its a very simple solution and covers all problems mentioned here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already covered by my pull request.

If I understand the pull request correctly, it only covers the endian case of LEGACY_UDP_CONNECT_REPLY but not the endian case of UDP_CONNECT_REPLY, which is what my suggested changes cover.

but the iperf3 maintainer seem to ignore all pull requests and bug fixes since a long time.

This may be because the iperf3 team was busy with releasing the multi-thread version. As this issue seems to be a major issue (that was caused because of my pull request ...) I hope that it (or a version of it) will be merged soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but there is another pull request made by me which covers everything. i posted it many months ago. this one here is not from me and was also posted a long time ago

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Thanks. I assume you are referring to PR #1468. By the way, I see that #1564 is another suggested PR to fix the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following is copied from the first comment in this PR...

Accept UDP connect message replies in little-endian or big-endian order.
There was discussion and concern about accepting two values in the original pull request #1260, this makes that problem worse. This is a solution that allows mixed-endian and mixed-version operations if the client is updated.

It's been over a year since I put this together so I really don't remember the specifics of what I found from the original change PR, but it was reasonable enough to make me try to limit this change. I work in an environment where updating the client is much more reasonable and frequent than updating the server. 3.12 is broken for mixed endian no matter what so something has to be updated. I was trying to limit the scope of damage that I was re-introducing that #1260 was attempting to fix. Since the author of #1260 is now part of the conversation I can assume he has a better handle on how many issues fixing everything will introduce.

If those concerns aren't real I also prefer a solution that addresses both client and server. That's an additional patch that I'm carrying on my product on top of this one since I'm unaware of ever having #1260 occur and cross-endian and version compatibility is important to me.

i_errno = IESTREAMREAD;
return -1;
}
Expand Down