#54 Segfault with SetEncoding

[Ralf Engels]

In convert_i the function iconv is called which
modifies the supplied pointers.
So freeing them afterwards produces a segfault.

The patch remembers the old pointers. In addition it
allocates the target buffer on the heap.

Patch was made for the cvs-stable version.

[Ralf Engels]

diff

[T.H.F. Klok]

Logged In: YES
user_id=564388

Yup, i see what you mean. But this code requires some tests,
since iconv is not the same everywhere. I've had some
problems compiling it on solaris before for instance.

if ID3LIB_ICONV_OLDSTYLE is defined then the assignment
char* source_buf = source_str;
will fail. (const char * -> char *)

Where does it segfault? target_str is only freed after it has
been copied to target (using append)

[Ralf Engels]

Logged In: YES
user_id=796299

Hi,
it segaults on the line delete target;
after iconv changed htis pointer.

I guess that there is no need to make tests on other
architectures because the iconv call syntax is not changed.

[T.H.F. Klok]

Logged In: YES
user_id=564388

It fails when ID3LIB_ICONV_OLDSTYLE is defined as i
explained earlier. All patches are off course tested on all
architectures, because most people suppling patches do not
take other architectures into account. Your patch will fail
compilation on systems where ID3LIB_ICONV_OLDSTYLE is
defined because of the cast from const char * to char *

[Ralf Engels]

Logged In: YES
user_id=796299

Hi,
of course you are right with both comments.

Making tests on all platforms is a good thing, I just wanted
to point out that this is not needed because of a change for
iconv.

The other point is also correct, the cast will fail.

The seg-fault appeared on the line
delete [] source_str;
becaus delete doesn't like to free a pointer that was not
allocated before.

Example you allocate a pointer and fill it with "Test"0
after iconv source_str points to the 0 (the termination).

The segfault did not longer appear after the fix.

[Ralf Engels]

Logged In: YES
user_id=796299

For you convinience:

(gdb) bt
#0 0x400a0052 in pthread_mutex_lock () from
/lib/libpthread.so.0
#1 0x40161386 in free () from /lib/libc.so.6
#2 0x403602a3 in operator delete(void*) (ptr=0x400a0040) at
del_op.cc:39
#3 0x403602ff in operator delete[](void*) (ptr=0x400a0040)
at del_opv.cc:36
#4 0x402b70f3 in (anonymous namespace)::convert_i(void*,
std::string) (cd=0x8315e48, source=
{static npos = 4294967295, _M_dataplus =
{<allocator<char>> = {<No data fields>}, _M_p = 0x8302c3c
"Pudge"}, static _S_empty_rep_storage = {0, 0, 7, 0}}) at
utils.cpp:148
#5 0x402b725c in dami::convert(std::string, ID3_TextEnc,
ID3_TextEnc) (data=
{static npos = 4294967295, _M_dataplus =
{<allocator<char>> = {<No data fields>}, _M_p = 0x8302c3c
"Pudge"}, static _S_empty_rep_storage = {0, 0, 7, 0}},
sourceEnc=ID3TE_ISO8859_1, targetEnc=ID3TE_UTF16) at
utils.cpp:206
#6 0x4029b6ab in ID3_FieldImpl::SetEncoding(ID3_TextEnc)
(this=0x816efb8, enc=ID3TE_UTF16) at field.cpp:1182
#7 0x4026d61c in
XS_MP3__ID3Lib__ID3_Field_Set(interpreter*, cv*) ()
from
/usr/lib/perl5/site_perl/5.8.1/i586-linux-thread-multi/auto/MP3/ID3Lib/ID3Lib.so
#8 0x080c545a in Perl_pp_entersub ()
#9 0x080bdc79 in Perl_runops_standard ()
#10 0x080634f8 in S_run_body ()
#11 0x08063365 in perl_run ()
#12 0x0805f9af in main ()

[Ralf Engels]

Logged In: YES
user_id=796299

And again a thing:

Error in my patch.
Instead of
+ target.append(target_buf, ID3LIB_BUFSIZ - target_size);
it should be
+ target.append(target_str, ID3LIB_BUFSIZ - target_size);

Sorry about this.