64 bit crash fix (undefined union assignment)
Good news, everyone! According to ISO C, assigning (i.e. "a = b;") one union to another only has to copy the "active" member of the union, that is, the last-accessed one. This means that if we assign unions with different active members, the behaviour is undefined because the wrong bytes will be copied; indeed, it crashes on (some!) 64-bit builds for this reason. The attached patch fixes the problem, and as far as I can tell I've fixed it in all relevant locations in librtmp. It should be noted that it would be nice if we had some kind of discipline with respect to this in the future, considering that the addition of a union to any existing struct we assign will generate the same problem. This is made worse by the fact that this is the kind of stuff that you can't grep for and have to inspect manually, and that the side effects never manifest at the exact "wrong" location (i.e. the assignment itself), but instead further down the line as the program hopelessly tries to access pointers that were "half-copied", etc. To illustrate, this particular problem took about an hour to find; an hour I could have been wasting instead. K
On Fri, Sep 30, 2011 at 12:52 PM, Kirill Zorin wrote:
indeed, it crashes on (some!) 64-bit builds for this reason.
Excuse my ignorance, but since when is rtmpdump 64 bit? The builds I have are all 32 bit, I grepped the source and didnt see anything.
Steven Penny wrote:
On Fri, Sep 30, 2011 at 12:52 PM, Kirill Zorin wrote:
indeed, it crashes on (some!) 64-bit builds for this reason.
Excuse my ignorance, but since when is rtmpdump 64 bit? The builds I have are all 32 bit, I grepped the source and didnt see anything.
That's because in well-written code, there is nothing to see. Word-size is purely a compiler issue. When you build on a 64 bit Linux system, the compiler defaults to 64 bit object code.
On Fri, Sep 30, 2011 at 1:17 PM, Howard Chu <hyc@highlandsun.com> wrote:
That's because in well-written code, there is nothing to see. Word-size is When you build on a 64 bit Linux system, the compiler defaults to 64 bit object code.
So the source has yet to be ported to win64?
Steven Penny wrote:
On Fri, Sep 30, 2011 at 1:17 PM, Howard Chu<hyc@highlandsun.com> wrote:
That's because in well-written code, there is nothing to see. Word-size is When you build on a 64 bit Linux system, the compiler defaults to 64 bit object code.
So the source has yet to be ported to win64?
There is nothing to "port". Build with 64-bit compiler switches, get a 64 bit binary.
Hi, I found this bug about a month ago, and posted a patch to this mailing list. You could have saved your hour if you had read my post. On Fri, Sep 30, 2011 at 12:52 PM, Kirill Zorin <cyril.zorin@gmail.com> wrote:
Good news, everyone!
According to ISO C, assigning (i.e. "a = b;") one union to another only has to copy the "active" member of the union, that is, the last-accessed one. This means that if we assign unions with different active members, the behaviour is undefined because the wrong bytes will be copied; indeed, it crashes on (some!) 64-bit builds for this reason. The attached patch fixes the problem, and as far as I can tell I've fixed it in all relevant locations in librtmp.
It should be noted that it would be nice if we had some kind of discipline with respect to this in the future, considering that the addition of a union to any existing struct we assign will generate the same problem. This is made worse by the fact that this is the kind of stuff that you can't grep for and have to inspect manually, and that the side effects never manifest at the exact "wrong" location (i.e. the assignment itself), but instead further down the line as the program hopelessly tries to access pointers that were "half-copied", etc. To illustrate, this particular problem took about an hour to find; an hour I could have been wasting instead.
K
_______________________________________________ rtmpdump mailing list rtmpdump@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/rtmpdump
-- --------------------- Gershon Bialer
Didn't get merged for some reason. If your patches don't get merged, you need to follow up. I don't usually comb the mailing list in hopes of finding unmerged patches, especially considering most posts are the "help me do this trivial thing; I've tried nothing and I'm all out of ideas"-kinds of posts. K On 2011-10-01, at 0:08, gershon bialer <gershon.bialer@gmail.com> wrote:
Hi,
I found this bug about a month ago, and posted a patch to this mailing list. You could have saved your hour if you had read my post.
On Fri, Sep 30, 2011 at 12:52 PM, Kirill Zorin <cyril.zorin@gmail.com> wrote:
Good news, everyone!
According to ISO C, assigning (i.e. "a = b;") one union to another only has to copy the "active" member of the union, that is, the last-accessed one. This means that if we assign unions with different active members, the behaviour is undefined because the wrong bytes will be copied; indeed, it crashes on (some!) 64-bit builds for this reason. The attached patch fixes the problem, and as far as I can tell I've fixed it in all relevant locations in librtmp.
It should be noted that it would be nice if we had some kind of discipline with respect to this in the future, considering that the addition of a union to any existing struct we assign will generate the same problem. This is made worse by the fact that this is the kind of stuff that you can't grep for and have to inspect manually, and that the side effects never manifest at the exact "wrong" location (i.e. the assignment itself), but instead further down the line as the program hopelessly tries to access pointers that were "half-copied", etc. To illustrate, this particular problem took about an hour to find; an hour I could have been wasting instead.
K
_______________________________________________ rtmpdump mailing list rtmpdump@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/rtmpdump
-- --------------------- Gershon Bialer _______________________________________________ rtmpdump mailing list rtmpdump@mplayerhq.hu https://lists.mplayerhq.hu/mailman/listinfo/rtmpdump
participants (4)
-
gershon bialer -
Howard Chu -
Kirill Zorin -
Steven Penny