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.

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

Segfault in client when specifying large buffer length with UDP #212

Closed
mdunphy opened this issue Oct 8, 2014 · 6 comments
Closed

Segfault in client when specifying large buffer length with UDP #212

mdunphy opened this issue Oct 8, 2014 · 6 comments
Assignees
Labels

Comments

@mdunphy
Copy link

mdunphy commented Oct 8, 2014

The client segfaults on the second attempt when specifying a buffer length too large in UDP mode. To reproduce,

  1. Start the server with "./iperf3 -s"

  2. Start the client with "./iperf3 -c SERVER -u -l 1M", see this message:

Connecting to host SERVER, port 5201
[ 4] local my.ip port 33697 connected to remote.ip port 5201
iperf3: error - unable to write to stream socket: Message too long

  1. Try it again, "./iperf3 -c SERVER -u -l 1M"

  2. Observe that there's nothing written to stdio/stderr, so press Ctrl+C, which yields a segfault
    ^C- - - - - - - - - - - - - - - - - - - - - - - - -
    [ ID] Interval Transfer Bandwidth Jitter Lost/Total Datagrams
    Segmentation fault (core dumped)

Repeating steps 3-4 will consistently generate a segfault. If you restart the server, the client's first attempt will give the "message too long" error once, and fall back to ctrl+c-segfaults on subsequent attempts.

Valgrind at the client output below. It looks like iperf3 as client is trying to write the report, but since no tests occurred, the variables it needs are not initialised and it implodes.

Since restarting the server fixes it, it seems like the server is remembering part of the state from the initial connection attempt, where it SHOULD be discarding it, and somehow this confuses the client in future attempts. So despite being a client segfault, it's probably a server bug.

Cheers,
Michael

$ valgrind .libs/lt-iperf3 -c SERVER -u -l 1M
==1481== Memcheck, a memory error detector
==1481== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==1481== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==1481== Command: .libs/lt-iperf3 -c SERVER -u -l 1M
==1481==
^C- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bandwidth Jitter Lost/Total Datagrams
==1481== Invalid read of size 8
==1481== at 0x4E3FAF1: iperf_reporter_callback (iperf_api.c:2183)
==1481== by 0x4E3E9C5: iperf_got_sigend (iperf_api.c:2672)
==1481== by 0x4E42A5A: iperf_run_client (iperf_client_api.c:351)
==1481== by 0x400B51: main (main.c:155)
==1481== Address 0x20 is not stack'd, malloc'd or (recently) free'd
==1481==
==1481==
==1481== Process terminating with default action of signal 11 (SIGSEGV)
==1481== Access not within mapped region at address 0x20
==1481== at 0x4E3FAF1: iperf_reporter_callback (iperf_api.c:2183)
==1481== by 0x4E3E9C5: iperf_got_sigend (iperf_api.c:2672)
==1481== by 0x4E42A5A: iperf_run_client (iperf_client_api.c:351)
==1481== by 0x400B51: main (main.c:155)

@mdunphy
Copy link
Author

mdunphy commented Oct 8, 2014

A couple more details: this is using the source cloned from the git repo today, not an old version. Also, linux 64bit at both server and client.

@bmah888 bmah888 added the bug label Oct 11, 2014
@bmah888 bmah888 self-assigned this Oct 11, 2014
@bmah888
Copy link
Contributor

bmah888 commented Oct 11, 2014

I confirm seeing this on my main development platform, which is MacOS 10.9 (master codeline) and FreeBSD 9.x (3.0-STABLE branch). It feels like there are actually multiple bugs at play here but I haven't really dug into this yet.

@bmah888
Copy link
Contributor

bmah888 commented Oct 13, 2014

I know of at least three bugs going on here:

  1. We shouldn't allow a send length of 1M for UDP because that's (much) larger than the maximum UDP datagram size.
  2. The server shouldn't get itself into a weird state.
  3. The client shouldn't crash if we interrupt it while it's trying to connect to the server in this weird state.

At the moment I seem to have at least a partial fix for the third problem. I presume the first problem is easy to fix, that's just argument validation. What's left is really the second problem, which I haven't looked at yet. The client really shouldn't mess up the server, so this is actually kind of important to fix.

@mdunphy : If I didn't say so already, thanks for a good bug report. You showed how to reproduce the problem (and followed up with important details about version and platform). Also you had some preliminary analysis of the bug, and you distinguished between what you observed directly and what you inferred.

bmah888 added a commit that referenced this issue Oct 13, 2014
This can happen if the server gets into a weird state (see the test
cases for reproducing issue #212).  We need to do a couple of checks
to make sure we're not dereferencing NULL pointers (yay C).

While here, also fix up a couple of related output glitches, where
in this case we can emit some invalid JSON (NaN values, such as what
you get if there's a division by zero, are not valid JSON).

Part of a fix in progress for #212.
@bmah888 bmah888 added this to the 3.0.x milestone Oct 13, 2014
bmah888 added a commit that referenced this issue Oct 13, 2014
…able.

We need this to permit a UDP receiving iperf3 server to listen on its
control channel.

The fix for non-blocking sockets basically makes sure that if we do a
read on a non-blocking sockets, but there's no data, the UDP processing
code does *not* try to do the normal protocol packet processing on the
non-existent packet.

This is part of an ongoing fix for issue #212 but also should have been
a part of the fix for issue #125.
@bmah888
Copy link
Contributor

bmah888 commented Oct 13, 2014

At this point the tip of the master branch has fixes for parts 2 and 3 of this bug. Part 2 is really a leftover bit of work from Issue #125.

bmah888 added a commit that referenced this issue Oct 13, 2014
For UDP over IPv4, this is the maximum IPv4 packet size (65535) minus
the size of the IPv4 and UDP headers, arriving at 65507.

In theory for a system implementing IPv6 jumbogram support, there is
no maximum packet size for UDP.  In practice we've observed with
CentOS 5 a limitation of 65535 - 8, which is dictated by the size
field in the UDP header (it has a maximum value of 65535, but needs
to count both payload and header bytes, thus subtracting off the 8
bytes for the UDP header).

We take the most conservative approach and use the 65507 value
for UDP / IPv4.

This is (I believe) the last part of issue #212.
@bmah888 bmah888 added the merge label Oct 13, 2014
@bmah888
Copy link
Contributor

bmah888 commented Oct 13, 2014

When we have a warm fuzzy feeling about the three commits for this issue, we want to get them merged to the 3.0-STABLE codeline.

@mdunphy
Copy link
Author

mdunphy commented Oct 13, 2014

Three bug fixes for the price of one report, not bad!

bmah888 added a commit that referenced this issue Oct 14, 2014
This can happen if the server gets into a weird state (see the test
cases for reproducing issue #212).  We need to do a couple of checks
to make sure we're not dereferencing NULL pointers (yay C).

While here, also fix up a couple of related output glitches, where
in this case we can emit some invalid JSON (NaN values, such as what
you get if there's a division by zero, are not valid JSON).

Part of a fix in progress for #212.

(cherry picked from commit 78033c4)
Signed-off-by: Bruce A. Mah <bmah@es.net>
bmah888 added a commit that referenced this issue Oct 14, 2014
…able.

We need this to permit a UDP receiving iperf3 server to listen on its
control channel.

The fix for non-blocking sockets basically makes sure that if we do a
read on a non-blocking sockets, but there's no data, the UDP processing
code does *not* try to do the normal protocol packet processing on the
non-existent packet.

This is part of an ongoing fix for issue #212 but also should have been
a part of the fix for issue #125.

(cherry picked from commit 8694d1d)
Signed-off-by: Bruce A. Mah <bmah@es.net>
bmah888 added a commit that referenced this issue Oct 14, 2014
For UDP over IPv4, this is the maximum IPv4 packet size (65535) minus
the size of the IPv4 and UDP headers, arriving at 65507.

In theory for a system implementing IPv6 jumbogram support, there is
no maximum packet size for UDP.  In practice we've observed with
CentOS 5 a limitation of 65535 - 8, which is dictated by the size
field in the UDP header (it has a maximum value of 65535, but needs
to count both payload and header bytes, thus subtracting off the 8
bytes for the UDP header).

We take the most conservative approach and use the 65507 value
for UDP / IPv4.

This is (I believe) the last part of issue #212.

(cherry picked from commit 96d0c77)
Signed-off-by: Bruce A. Mah <bmah@es.net>

Conflicts:
	src/iperf_api.h
	src/iperf_error.c
@bmah888 bmah888 removed the merge label Oct 14, 2014
@bmah888 bmah888 closed this as completed Oct 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants