openigtlink / OpenIGTLink

Free, open-source network communication library for image-guided therapy
http://openigtlink.org
BSD 3-Clause "New" or "Revised" License
102 stars 184 forks source link

igtlMessageBase : bodySizeToRead is signed int 32, but header is unsigned int 64 #244

Closed NicolasCourtial closed 3 years ago

NicolasCourtial commented 3 years ago

Hi everyone,

I'm facing an issue regarding the igtlMessageBase class, particularly in terms of message content.

In documentation and in igtl_header I read that body size is given as a uint64. So I'd expect to be able to send messages whose payload can be as heavy as 2^45 bytes for example (what my sister need to send her dog's pictures).

However, m_BodySizeToRead is stored as a signed int 32, and then use to allocate buffers and all, meaning my payload can't exceed 2^31 -1 bytes.

Is there a reason for it?

adamrankin commented 3 years ago

No reason, I suspect no one has hit that limit yet.

adamrankin commented 3 years ago

Just to confirm, you're trying to send an image that is 35TB in size?

leochan2009 commented 3 years ago

Yes, we didn't expect to send data of that size within one message.

NicolasCourtial commented 3 years ago

Thanks for your quick answer ! The 35TB was more a joke, but yes, I expect to reach the 2.2GB limit quite soon, as we are sending archives containing multiple patient images. We've been considering to send data with multiple messages, but that would definitely break a lot of logic then.

Looking at your commit, I'd say some uint64 -> int cast are still quite dangerous, few examples :

adamrankin commented 3 years ago

@leochan2009 Ultimately the socket class uses recv, which is limited to 32bit. Should we change the spec?

Edit: we can put the send and recv in a loop to send/receive until the desired amount is reached

leochan2009 commented 3 years ago

i prefer changing the specs. To send a message with size more than 2^31 is not intended and can be risky if connection is not stable and socket will try to resend.

NicolasCourtial commented 3 years ago

Sure. Could that be just unsigned int 32? if 2^32-1 is acceptable?

leochan2009 commented 3 years ago

fixed with the commits

NicolasCourtial commented 3 years ago

Thanks!

tokjun commented 3 years ago

The new 'timeout' argument for igtl::Socket::Recieve() in b463c7f causes building errors in existing codes including Examples/Imager/ImagerServer3. Maybe we add default value (timeout=false)?

adamrankin commented 3 years ago

It is an "out" variable. I'll do another pass at the missed locations.

tokjun commented 3 years ago

Sorry, adding a default value wouldn't work since timeout is a reference. But I'm still not sure why this new argument is needed. The comment says Receive returns -1 on timeout. Recieve() seems to return always zero, when timeout is set to true.

adamrankin commented 3 years ago

The new return type is igtlUint64, so a value of -1 is not possible. Updated comment to match new behaviour.

tokjun commented 3 years ago

Ah, I see.