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

incoming UDP packets truncated to 4096 bytes (theoretical maximum is ~65535 bytes) #903

Open
umlaeute opened this issue Mar 6, 2020 · 8 comments
Labels

Comments

@umlaeute
Copy link
Contributor

@umlaeute umlaeute commented Mar 6, 2020

given a patch:

[netreceive -u -b 9000]
|
[list length]
|
[print]

resp.

#N canvas 5 51 450 300 12;
#X obj 102 75 netreceive -u -b 9000;
#X obj 102 100 list length;
#X obj 102 125 print;
#X connect 0 0 1 0;
#X connect 1 0 2 0;

if i feed this patch 2048 bytes using the following command, it will only receive 1000.

$ dd if=/dev/zero bs=2048 count=1 | nc -u localhost 9000

if i instead replace [netreceive -u -b 9000] with iemnet's [udpreceive 9000], i receive the 2048 bytes just fine.

this might be a regression triggered by #715

@umlaeute umlaeute added the bug/fix label Mar 6, 2020
@Spacechild1

This comment has been minimized.

Copy link
Contributor

@Spacechild1 Spacechild1 commented Mar 6, 2020

Can't reproduce here (on Windows). Are you sure that you're using current master? I actually increased the buffer for binary netreceive from MAXPDSTRING (=1000) to INBUFSIZE (=4096), so that binary and FUDI have the same size restriction.

With current master I can send/receive UDP packets up to 4096 bytes. Above that, Pd will segfault (which is another issue, I'll push a fix).

@umlaeute

This comment has been minimized.

Copy link
Contributor Author

@umlaeute umlaeute commented Mar 6, 2020

i was using Pd-0.50-2 (not master), and can confirm that with master the limit is 4096 bytes.
it doesn't crash here if i send larger packages (and valgrind only complains about a few lost bytes but no other memory problems), the data is just truncated at 4096 bytes.

however: a UDP package has a possible maximum length of 65535 bytes. it there any reason to not allow this maximum size?

the original issue (limit=1000) is therefore already fixed. however, since the follow-up issue (limit=4096) is almost identical, i'll leave this ticket open and re-title it.

@umlaeute umlaeute changed the title incoming UDP packets truncated to 1000 bytes incoming UDP packets truncated to 4096 bytes (theoretical maximum is 65535 bytes) Mar 6, 2020
@umlaeute umlaeute changed the title incoming UDP packets truncated to 4096 bytes (theoretical maximum is 65535 bytes) incoming UDP packets truncated to 4096 bytes (theoretical maximum is 65507 bytes) Mar 6, 2020
@umlaeute umlaeute changed the title incoming UDP packets truncated to 4096 bytes (theoretical maximum is 65507 bytes) incoming UDP packets truncated to 4096 bytes (theoretical maximum is ~65535 bytes) Mar 6, 2020
@Spacechild1

This comment has been minimized.

Copy link
Contributor

@Spacechild1 Spacechild1 commented Mar 7, 2020

it there any reason to not allow this maximum size?

I think it just makes the implementation easier. Having a 65KB buffer on the stack doesn't feel right. There are of course other solutions:
a) query the size of the packet in advance with socket_bytes_available + alloca
b) allocate a single shared buffer in _instanceinter, just like i_inbinbuf

On the other hand, the limit for binary UDP packets has been 1000 bytes like forever and nobody seemed to notice, let alone complain. Now the limit is 4 times as high. Of course the theoretical limit for UDP packets is 65535 bytes, but in practice it's not recommended to send UDP packets larger than the network MTU. I'm not against supporting larger UDP packets, but maybe wait till someone really needs it?

I've pushed some code which at least gives you a warning that packets have been truncated (3d8aeb4) and also fixes a crasher bug on Windows (which was caused by recv returning an error, see f2929e3).

@danomatika Maybe doublecheck.

@Spacechild1

This comment has been minimized.

Copy link
Contributor

@Spacechild1 Spacechild1 commented Mar 7, 2020

Actually, Scsynth and Supernova both use a heap buffer of 65535 bytes for UDP packets, so why not?

@umlaeute

This comment has been minimized.

Copy link
Contributor Author

@umlaeute umlaeute commented Mar 7, 2020

also note that this issue was triggered by a real-world problem, where [netreceive] could not receive a large OSC-bundle.

@umlaeute

This comment has been minimized.

Copy link
Contributor Author

@umlaeute umlaeute commented Mar 17, 2020

@Spacechild1 since you pushed related stuff recently, do you think we should also raise the INBUFSIZE?

@Spacechild1

This comment has been minimized.

Copy link
Contributor

@Spacechild1 Spacechild1 commented Mar 17, 2020

Instead of having a huge buffer sitting on the stack, I would rather allocate 65536 bytes once in _instanceinter and use that in all netreceivers, roughly following the approach of Supercollider. I can make a PR.

@umlaeute

This comment has been minimized.

Copy link
Contributor Author

@umlaeute umlaeute commented Mar 17, 2020

that'd be cool.

in practice it's not recommended to send UDP packets larger than the network MTU.

true, but on my loopback interface i have an MTU of 65535 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.