Wed 30 Apr 2008 06:33:32 PM UTC, comment #23:
Boundary checkign in AMF parser made the bug more visible.
The AMF parser now throws an exception which is interchepted
by the SOL writer which logs an error and goes on.
The test give no unexpected failures now, just normal
ones, and run valgrind-clean (didn't check leaks, which I'm sure
would be introduced by throwing exception, since we don't
do inner catches and don't use smart pointers in the parser).
|
Tue 15 Apr 2008 01:33:33 PM UTC, comment #22:
I plan to continue to improve all the test cases that utilize AMF objects. I'm working my way up from the bottom (buffer) to the top (test_sol, test_lc). Then I can go back to RTMP hacking, which also uses this code, but it might as well be solid and well tested. I get no errors ever myself, so I'm curious what the differences could. Quite a few of the bugs I've been fixing are compiler issues, works on some machines but not all. So then I tweak the code to work on all platforms. Much of the type casting between data types can have issues with varying compiler versions.
|
Tue 15 Apr 2008 07:49:50 AM UTC, comment #21:
Just to confirm the intermittency.
Still get same report when I get one.
Threading can be completely removed by commenting out
the LOAD_MOVIES_IN_A_SEPARATE_THREAD define in server/parser/movie_def_impl.cpp.
Other things that could trigger the intermittency I can
think of are:
- Load of the external .sol file, which can be different
each run (as IIRC each run writes it)
- Statics initialization/destruction order [ not sure these
would be unpredictable though)
|
Mon 14 Apr 2008 03:29:39 PM UTC, comment #20:
I'm working on expanded test cases now for the encoding of AMF objects, which will be followed by much more detailed and invasive test cases for extracting AMF objects. The test_sol.cpp test case itself is a bit funky, it also needs work. I plan to spend several more days at this effort (while traveling though) of adding new test cases to libamf to wring out any potential problems. After the low level test cases are done for encoding & decoding, I can see about improving test_sol and test_lc.
Writing good, accurate, and detailed tests for things that play with memory buffers is boring, tedious, but will be good when it's all done.
|
Mon 14 Apr 2008 03:17:20 PM UTC, comment #19:
This can't be right, though:
47895696692400] 17:11:18 DEBUG: element name size: 4
47895696692400] 17:11:18 DEBUG: AMF element name is: gain
47895696692400] 17:11:18 DEBUG: element name size: 15
47895696692400] 17:11:18 DEBUG: AMF element name is: echosuppression
47895696692400] 17:11:18 DEBUG: element name size: 210
47895696692400] 17:11:18 DEBUG: AMF element name is:
47895696692400] 17:11:18 DEBUG: element name size: 4
47895696692400] 17:11:18 DEBUG: AMF element name is: gain
47895696692400] 17:11:18 DEBUG: element name size: 15
47895696692400] 17:11:18 DEBUG: AMF element name is: echosuppression
47895696692400] 17:11:18 DEBUG: element name size: 210
47895696692400] 17:11:18 DEBUG: AMF element name is:
(tracing size in Element::setName(Network::byte_t *name, size_t size)). The value of size, particularly the last call, varies a lot.
The size of the test SOL files written varies unpredictably, and the number of times that extractVariable is called when running SharedObject.as varies between 2 and 6, which suggests either a bug or a not very consistent set of tests.
|
Mon 14 Apr 2008 01:59:47 PM UTC, comment #18:
Intermittent is right. I checked in a fix last night that fixed this for me, but before then it was hard to reproduce. I suspect it's actually not a bug in the SOL/AMF code, but one in Gnash that this is triggering. For a while it only failed for me with v7.swf output of the test case, but worked 100% for the other versions.
All the lower level test cases in libamf after repeated attempts show no problems with Valgrind or mudflap at all.As these tests are run in isolation from the rest of Gnash, I suspect the bug may be elsewhere. (threading problem)
|
Mon 14 Apr 2008 12:45:05 PM UTC, comment #17:
I'm still getting a very intermittent fault with Shared Object. The actionscript.all SharedObject.as test fails (gprocessor returns an error) occasionally.
The actual failure appears to be construction of a string from NULL, but as it's so difficult to reproduce I'm not sure that's always the case.
Valgrind also only rarely reports an error, which makes it particularly hard to debug, but when it does, it's the same as strk reported in Comment #12.
|
Tue 08 Apr 2008 03:34:23 PM UTC, comment #16:
Arg... on my local machine I've been statically linking, so didn't need to use libtool execute mode. On the debian test machine I was building dynamic libraries by default.
|
Tue 08 Apr 2008 03:29:57 PM UTC, comment #15:
Correct call:
GNASHRC=../gnashrc libtool --mode=execute valgrind ../../utilities/gprocessor SharedObject-v7.swf
It also neds GNASHRC to properly set the SAFESolDir
|
Tue 08 Apr 2008 03:29:04 PM UTC, comment #14:
Rob, you need libtool --mode=execute, w/out that your run
is debugging the bourne shell.
Was a common error of myself. You can tell by the multiple
ERROR SUMMARY lines, there should be only one for a correct
run.
|
Tue 08 Apr 2008 03:24:11 PM UTC, comment #13:
testsuite/actionscript.all{rob@x86-debianetch} pts/0> valgrind ../../utilities/gprocessor SharedObject-v7.swf
==30589== Memcheck, a memory error detector.
==30589== Copyright (C) 2002-2006, and GNU GPL'd, by Julian Seward et al.
==30589== Using LibVEX rev 1658, a library for dynamic binary translation.
==30589== Copyright (C) 2004-2006, and GNU GPL'd, by OpenWorks LLP.
==30589== Using valgrind-3.2.1-Debian, a dynamic binary instrumentation framework.
==30589== Copyright (C) 2000-2006, and GNU GPL'd, by Julian Seward et al.
==30589== For more details, rerun with: -v
==30589==
==30591==
==30591== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30591== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30591== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30591== For counts of detected errors, rerun with: -v
==30591== All heap blocks were freed -- no leaks are possible.
==30590==
==30590== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30590== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30590== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30590== For counts of detected errors, rerun with: -v
==30590== All heap blocks were freed -- no leaks are possible.
==30592==
==30592== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30592== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30592== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30592== For counts of detected errors, rerun with: -v
==30592== All heap blocks were freed -- no leaks are possible.
==30594==
==30594== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30594== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30594== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30594== For counts of detected errors, rerun with: -v
==30594== All heap blocks were freed -- no leaks are possible.
==30593==
==30593== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30593== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30593== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30593== For counts of detected errors, rerun with: -v
==30593== All heap blocks were freed -- no leaks are possible.
==30596==
==30596== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30596== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30596== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30596== For counts of detected errors, rerun with: -v
==30596== All heap blocks were freed -- no leaks are possible.
==30595==
==30595== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30595== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30595== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30595== For counts of detected errors, rerun with: -v
==30595== All heap blocks were freed -- no leaks are possible.
==30598==
==30598== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30598== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30598== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30598== For counts of detected errors, rerun with: -v
==30598== All heap blocks were freed -- no leaks are possible.
==30601==
==30601== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30601== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30601== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30601== For counts of detected errors, rerun with: -v
==30601== All heap blocks were freed -- no leaks are possible.
==30602==
==30602== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 15 from 1)
==30602== malloc/free: in use at exit: 0 bytes in 0 blocks.
==30602== malloc/free: 0 allocs, 0 frees, 0 bytes allocated.
==30602== For counts of detected errors, rerun with: -v
==30602== All heap blocks were freed -- no leaks are possible.
Any segfault past this message is likely due to improper threads cleanup.
|
Mon 07 Apr 2008 08:23:40 AM UTC, comment #12:
Alright, finally finished building. SharedObject.as run give
no failure but still trigger invalid memory access.
Valgrind report follows:
==3766== Invalid read of size 1
==3766== at 0x4962954: amf::Element::setName(unsigned char*, unsigned) (stl_algobase.h:285)
==3766== by 0x49523B7: amf::AMF::extractVariable(unsigned char*) (amf.cpp:1233)
==3766== by 0x497208C: amf::SOL::readFile(std::string&) (sol.cpp:377)
==3766== by 0x436F5CC: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:395)
==3766== by 0x4148BBA: gnash::builtin_function::operator()(gnash::fn_call const&) (builtin_function.h:87)
==3766== by 0x44BD738: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, int, int, gnash::as_object*) (action.cpp:157)
==3766== by 0x44633BF: gnash::SWF::SWFHandlers::ActionCallMethod(gnash::ActionExec&) (ASHandlers.cpp:3628)
==3766== by 0x444B191: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:176)
==3766== by 0x444BBB4: gnash::SWF::SWFHandlers::execute(gnash::SWF::action_type, gnash::ActionExec&) const (ASHandlers.cpp:457)
==3766== by 0x44AC40D: gnash::ActionExec::operator()() (ActionExec.cpp:351)
==3766== by 0x41D46D0: gnash::GlobalCode::execute() (ExecutableCode.h:74)
==3766== by 0x41C4182: gnash::movie_root::processActionQueue(int) (movie_root.cpp:1361)
==3766== Address 0x540052c is 0 bytes after a block of size 228 alloc'd
==3766== at 0x4022F14: operator new[](unsigned) (vg_replace_malloc.c:268)
==3766== by 0x4971F2D: amf::SOL::readFile(std::string&) (sol.cpp:336)
==3766== by 0x436F5CC: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:395)
|
Mon 07 Apr 2008 07:32:06 AM UTC, comment #11:
We checked sizeof(string) saturday, on IRC. It is the same as sizeo(char*) with the GNU libc.
What's the improvement in using char* instead of std::string ?
std::string can be returned by const ref, which hasn't an higher
cost then returning a char *, but gives you proper copy-ctor and
the like.
Beside, it's still fine to use char* if you make sure copy constructor, assignment operator and destructors are correctly
defined as per conceptual ownership of the memory.
Bigger problem is in RTMP when you use the name of the element
(the char*) as a key for the map. That's a conceptual bug in
that a variable named "test" won't be found if you look for "test"
as the two pointers have different values (different memory pointers). You'll have to use strcmp in a custom comparator
for that variables lookup to work, or use std::string, which
will give you that by default but take up more memory, or
use a member function of the Element class to compare name-wise
(should be the most memory-saver).
|
Sun 06 Apr 2008 05:41:57 PM UTC, comment #10:
All the SharedObject test cases work for me now after my latest bug fixes were checked in yesterday afternoon.
I get this from SharedObject-v?.swf:
3086358864] 11:09:40 TRACE: #passed: 33
3086358864] 11:09:40 TRACE: #failed: 0
3086358864] 11:09:40 TRACE: #expected failures: 5
3086358864] 11:09:40 TRACE: #total tests run: 38
Btw, in normal usage, AMF objects don't have names. Local Shared Objects and Local Connections both use a "variable" extension of AMF, which is a length, the name, and then the regular AMF type, length, and finally the data.
I applied your patch for _name() vs _name(0), but it makes zero difference in my test runs. I also was using std::string for the _name, but using a char * is the improved version.
|
Wed 02 Apr 2008 09:35:47 PM UTC, comment #9:
Okay, this is weird. On Hardy, gnash crashes with this command:
gui/gnash -v http://gnashdev.org/testcases/v6/SharedObject-v6.swf
BUT it didn't crash the first time I tried it. strk asked me to run it with valgrind (which I had to install) and it segfaulted. Then I ran it without valgrind (as above) and the second time, it segfaulted.
Phase of the moon? Or uninitialized memory?
|
Wed 02 Apr 2008 09:31:16 PM UTC, comment #8:
Next patch fixes second problem, closing the SharedObject case.
element_rtmp.patch
It makes Element class store name by std::string
and fixes the variables container of RTMP class to
use actual string values, not memory pointer for
indexing (also adds comments suggesting a better implementation)
With this, valgrind reports NO error running SharedObject
tests. There are still others (segfaulting) in other libnet/rtmp/amf related testsuite.
I'll open other bug reports for them.
(file #15384)
|
Wed 02 Apr 2008 07:23:49 PM UTC, comment #7:
Question: is it acceptable for an AMF::Element to NOT have a name ?
or would it be an invariant violation ?
|
Wed 02 Apr 2008 07:17:07 PM UTC, comment #6:
Second problem is in SharedObject.cpp, where the return from
AMF::Element::getName() is passed to std::string constructor.
The return is NULL of course..
BTW, if an Element name is not null it would leak, as
the Element destructor doesn't delete the _name pointer.
My suggestion here is to use a std::string for Element::_name.
Should I provide a patch or will you take care of that rob ?
Backtrace follows:
terminate called after throwing an instance of 'std::logic_error'
what(): basic_string::_S_construct NULL not valid
Program received signal SIGABRT, Aborted.
[Switching to Thread -1223485760 (LWP 20269)]
0xffffe410 in __kernel_vsyscall ()
(gdb) bt
#0 0xffffe410 in __kernel_vsyscall ()
#1 0xb7964875 in raise () from /lib/tls/i686/cmov/libc.so.6
#2 0xb7966201 in abort () from /lib/tls/i686/cmov/libc.so.6
#3 0xb7b3f6e0 in __gnu_cxx::__verbose_terminate_handler () from /usr/lib/libstdc++.so.6
#4 0xb7b3cf65 in ?? () from /usr/lib/libstdc++.so.6
#5 0xb7b3cfa2 in std::terminate () from /usr/lib/libstdc++.so.6
#6 0xb7b3d0ca in __cxa_throw () from /usr/lib/libstdc++.so.6
#7 0xb7ad2d3f in std::__throw_logic_error () from /usr/lib/libstdc++.so.6
#8 0xb7b18c2f in ?? () from /usr/lib/libstdc++.so.6
#9 0xb7b18d77 in std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string () from /usr/lib/libstdc++.so.6
#10 0xb7dcf30c in gnash::sharedobject_getlocal (fn=@0xbfafd424) at /home/strk/src/gnash/gnash-head/server/asobj/SharedObject.cpp:414
|
Wed 02 Apr 2008 06:41:45 PM UTC, comment #5:
Patch number 1: AMF::extractVariable declares an Element pointer
but never constructs an Element object. The pointer happens to be initialized to 0 for me, but that's not guaranteed (memory corruption).
The patch uses a std::auto_ptr to be exception safe in case
something is thrown or an early return is hit in that function.
For my taste, the function should return the auto_ptr to make
it clear ownership is transferred, but that's up to you.
Attached: extractVariableUninitializedElement.patch
Next issue on next comment.
(file #15382)
|
Wed 02 Apr 2008 07:09:20 AM UTC, comment #4:
FAIL: test_el: Created string element
FAIL: test_sol died prematurely (segmentation violation)
=== Summary ===
# of expected passes 19
# of unexpected failures 2
make[1]: Leaving directory `/home/strk/src/gnash/build/head/allgui_agg_gst_debugging/testsuite/libamf.all'
I did add the assertions last time you couldn't reproduce
a problem with SharedObject (the uptoten.com case).
With assertion, we abort rather the write to uninitialized memory.
When code writes to uninitialized memory results are not consistent, so it might fail for me and succeed for you based
on luck. Please check with valgrind, as it should report memory
failure even if the tests succeed.
|
Wed 02 Apr 2008 01:05:46 AM UTC, comment #3:
# of expected passes 31
# of expected failures 1
make[1]: Leaving directory `/home/rob/projects/gnu/i686-pc-linux-gnu/gnash/testsuite/libamf.all'
I have no uncommited patches in libamf. test_sol has been core dumping for me for a week, now it works fine. Somebody had added asserts() everywhere, and somehow deleted calculating the size of the output buffer, so it crashed. That has been fixed now.
The http messages need to be fixed,they just need to catch up with all the other recent changes.
|
Wed 02 Apr 2008 12:51:49 AM UTC, comment #2:
All SWF version fail with same segfault.
Note that I did commit a patch to fix writeFile which
was writing back to the input Buffer rather then to the
output one for BOOLEAN elements. Found out that as the
code didn't build on 64 machines (append call was ambiguos).
Also note that SharedObject is not the only testcase failing
for amf/libnet. This is the rest:
--=[ ./testsuite/libamf.all ]=--
FAIL: test_el: Created string element
FAIL: test_sol died prematurely (segmentation violation)
--=[ ./testsuite/libnet.all ]=--
FAIL: test_http: HTTP::formatDate()
FAIL: test_http: HTTP::formatContentLength()
FAIL: test_http: HTTP::formatServer()
FAIL: test_http: HTTP::formatHost()
FAIL: test_http: HTTP::formatLanguage()
FAIL: test_http: HTTP::formatHeader(port)
Are you sure you don't have uncommitted patches there ?
|
Wed 02 Apr 2008 12:34:21 AM UTC, comment #1:
Well, the SharedObject.as testcase fine works for me, Fedora 7, gcc 4.2.1. Was it a problem with a specific swf version ? I'll go try a build on a Gutsy system, but you might want to make you didn't have any cvs or build problems.
Prior to my recent checkin this afternoon, the test_sol testcase was core dumping after changes earlier in the week. Right now HEAD has all the test cases that use AMF working fine.
|
Wed 02 Apr 2008 12:20:16 AM UTC, original submission:
==9192== Invalid write of size 4
==9192== at 0x46CE220: amf::AMF::extractVariable(unsigned char*) (element.h:129)
==9192== by 0x46DB639: amf::SOL::readFile(std::string&) (sol.cpp:377)
==9192== by 0x4204B65: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:395)
==9192== by 0x40D2A7A: gnash::builtin_function::operator()(gnash::fn_call const&) (builtin_function.h:87)
==9192== by 0x42B9EE8: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, int, int, gnash::as_object*) (action.cpp:157)
==9192== by 0x427A4DF: gnash::SWF::SWFHandlers::ActionCallMethod(gnash::ActionExec&) (ASHandlers.cpp:3626)
==9192== by 0x4268F21: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:176)
==9192== by 0x427E0B5: gnash::SWF::SWFHandlers::execute(gnash::SWF::action_type, gnash::ActionExec&) const (ASHandlers.cpp:457)
==9192== by 0x42B4AB4: gnash::ActionExec::operator()() (ActionExec.cpp:351)
==9192== by 0x411E32D: gnash::GlobalCode::execute() (ExecutableCode.h:74)
==9192== by 0x4116672: gnash::movie_root::processActionQueue(int) (movie_root.cpp:1351)
==9192== by 0x411671E: gnash::movie_root::processActionQueue() (movie_root.cpp:1418)
==9192== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==9192==
==9192== Process terminating with default action of signal 11 (SIGSEGV)
==9192== Access not within mapped region at address 0x0
==9192== at 0x46CE220: amf::AMF::extractVariable(unsigned char*) (element.h:129)
==9192== by 0x46DB639: amf::SOL::readFile(std::string&) (sol.cpp:377)
==9192== by 0x4204B65: gnash::sharedobject_getlocal(gnash::fn_call const&) (SharedObject.cpp:395)
==9192== by 0x40D2A7A: gnash::builtin_function::operator()(gnash::fn_call const&) (builtin_function.h:87)
==9192== by 0x42B9EE8: gnash::call_method(gnash::as_value const&, gnash::as_environment, gnash::as_object, int, int, gnash::as_object*) (action.cpp:157)
==9192== by 0x427A4DF: gnash::SWF::SWFHandlers::ActionCallMethod(gnash::ActionExec&) (ASHandlers.cpp:3626)
==9192== by 0x4268F21: gnash::SWF::ActionHandler::execute(gnash::ActionExec&) const (ASHandlers.cpp:176)
==9192== by 0x427E0B5: gnash::SWF::SWFHandlers::execute(gnash::SWF::action_type, gnash::ActionExec&) const (ASHandlers.cpp:457)
==9192== by 0x42B4AB4: gnash::ActionExec::operator()() (ActionExec.cpp:351)
==9192== by 0x411E32D: gnash::GlobalCode::execute() (ExecutableCode.h:74)
==9192== by 0x4116672: gnash::movie_root::processActionQueue(int) (movie_root.cpp:1351)
==9192== by 0x411671E: gnash::movie_root::processActionQueue() (movie_root.cpp:1418)
|