Opened 10 years ago

Closed 10 years ago

#4260 closed Bug (fixed)

µTP not working on embedded device

Reported by: KyleK Owned by: jordan
Priority: Normal Milestone: 2.40
Component: Transmission Version: 2.30
Severity: Major Keywords:
Cc:

Description

At least on the NAS devices Conceptronic CH3SNAS and DNS-323, µTP is not working. These devices run libuClibc 0.9.29, and Transmission was compiled with gcc 4.1.3.

Transmission does not crash, but the log is filled with "Unexpected UDP packet" messages.

The reason for this is that the structs defined in libutp don't have the correct size on this platform.

I've attached a patch to fix the problem, although I can't speak for other platforms, so this should be tested beforehand.

We actually found this issue a while ago during an IRC session (I believe livings was involved), and we found the solution right then and there. But apparently it was not checked into the trunk / reported upstream.

Attachments (2)

packed_structs.patch (2.8 KB) - added by KyleK 10 years ago.
packed-test-gnuc.diff (2.6 KB) - added by jordan 10 years ago.

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by KyleK

comment:1 Changed 10 years ago by jordan

  • Component changed from libtransmission to Transmission
  • Milestone changed from None Set to 2.32
  • Status changed from new to assigned
  • Version changed from 2.22+ to 2.30

comment:2 Changed 10 years ago by jordan

I've passed this along upstream: https://github.com/bittorrent/libutp/issues/19

comment:3 Changed 10 years ago by jordan

KyleK, that attribute won't work on some compilers/platforms that libutp needs to compile on. Please update the patch s.t. an #ifdef is used to see if the packed attribute is supported by the compiler.

alus from upstream suggests something along these lines:

#if defined BROKEN_GCC_STRUCTURE_PACKING && defined __GNUC__
// Used for gcc tool chains accepting but not supporting pragma pack
// See http://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html
#define PACKED_ATTRIBUTE __attribute__((__packed__))
#else
#define PACKED_ATTRIBUTE
#endif // defined BROKEN_GCC_STRUCTURE_PACKING && defined __GNUC__

comment:4 Changed 10 years ago by KyleK

Um, now you got me :/

I didn't actually come up with the contents of the patch, these changes were suggested by alus or livings back in February, when we first ran into this issue. I don't really know what it is actually doing.

Copying the above suggestion to templates.h disables packed structures when compiling, because BROKEN_GCC_STRUCTURE_PACKING is not set. I suppose this define is determined via (a yet-to-be-written) autoconf test?

comment:5 Changed 10 years ago by jordan

IMO checking for {{#ifdef GNUC}} would be sufficient.

attribute((packed)) has existed on GCC for over 15 years, so adding "BROKEN_GCC_STRUCTURE_PACKING" is pointless, and requires extra hoops in autoconf...

comment:6 Changed 10 years ago by jordan

KyleK, could you try out this attached patch and see if it works?

I notice that your patch packs a few structs that are declared outside of the original "#pragma pack(push,1)" ... "#pragma pack(pop)" block. I don't know if you packed the extra structs because it was necessary or because you were being thorough, so I've left those extra packings out of this patch just to see whether or not it will work :)

Changed 10 years ago by jordan

comment:7 Changed 10 years ago by KyleK

I was just being thorough when applying the packed attribute to structs. As I said before, I had no (real) clue what I was doing :)

That said, your patch doesn't correctly work as-is. gcc gives a warning that it cannot pack PacketFormat? and PacketFormatV1 because some members cannot be packed. When running Transmission, it is killed after an assertion is thrown (PacketFormatV1 doesn't have the correct size).

When adding the packed attribute to the big_endian template struct defined in templates.h, both the warnings and the assertion go away and everything works just fine.

comment:8 Changed 10 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

KyleK, r12466 contains packed-test-gnuc.diff plus the changes you mentioned in comment:7.

If I've missed anything, please reopen the ticket. Thanks.

comment:9 Changed 10 years ago by jch

Folks, please do *not* patch the Transmission copy of libutp without pushing the changes upstream. It makes everyone's life uselessly complicated.

https://github.com/bittorrent/libutp/issues/19#issuecomment-1688736

-- jch

comment:10 Changed 10 years ago by jordan

  • Resolution fixed deleted
  • Status changed from closed to reopened

reopening to test alus' modification.

KyleK, could you please test a nightly build and confirm that the issue is still solved by this revision?

comment:11 Changed 10 years ago by KyleK

It seems to me r12603 simply reverts the patch introduced with r12466, making UTP fail again on my NAS (although it doesn't crash, I get lots of "unrecognized IDP packet" messages instead).

As far as I can see the upstream ticket is still unresolved and the maintainer (alus?) hasn't made any changes to the codebase to fix the issue.

comment:12 Changed 10 years ago by jordan

KyleK, I erred in applying from the wrong repo. alus committed his change to https://github.com/ghazel/libutp for testing first.

I've applied that testing patch in r12663 for short-term testing. We'll resync with https://github.com/bittorrent/libutp after this is done.

Also, going through Transmission's svn repository is not the most direct way of doing this. Alus, if you're comfortable with making local changes to your sandbox from alus' repository -- either through diffs or simple file copying -- we could take Transmission's svn repository out of the loop.

comment:13 Changed 10 years ago by jordan

KyleK, any news?

comment:14 Changed 10 years ago by KyleK

Sorry, I've been away from the computer for a couple of days and couldn't respond.

Alus' patch works fine, just as expected.

comment:15 Changed 10 years ago by jordan

  • Milestone changed from 2.32 to 2.40
  • Resolution set to fixed
  • Status changed from reopened to closed

This is fixed now upstream @ https://github.com/bittorrent/libutp/commit/286cbe512af6280ca6dde4715359245eb497e9a1

There aren't any diffs between trunk's third-party/libutp files and those upstream, so marking this ticket as closed/fixed.

Note: See TracTickets for help on using tickets.