Fix CDTEXT processing #6

K3bDevice::CdText::textForPackType returns link to a destructed object.
Additionally, same issue was fixed for AudioEncoder module.

Fixes ~~https://bugs.trinitydesktop.org/show_bug.cgi?id=1550~~ https://bugs.trinitydesktop.org/show_bug.cgi?id=2307

IMHO, there is no reason to return a copy of a private object instead of a constant reference to it.

Hi Mashiro and welcome to TGW!!
Slavek is suggesting that instead of changing the function return type, we simply change the "return TQString()" instruction to a "return TQString::null" instruction. The null string always exists and therefore it is safe to return a reference to that object. Would you be able to try like that? If good then you can update/simplify the PR.

Could you also signoff your commit and force push it again so that later we can merge it? See here for instructions, especially the sentence in red color 😄

IMHO, there is no reason to return a copy of a private object instead of a constant reference to it.

Hi Denis,

in C/C++ you can't safely return a reference to an object allocated in the stack within the function you are returning from. You can only return a reference to an object that is either allocated somewhere else or in the heap. Otherwise you need to return a copy because the local object will be destroyed when leaving the function scope 😄

I did not see any stacked object in this code (by the way, C/C++ standard says about "automatic storage" instead of stack).
surprise :)

In both the functions in the PR, there is a point where TQString() is invoked and the reference to the new object returned. It is not visible in the commit, you have to open the files and check further in the code, but it is there 😄 which I guess is the reason of the original crash.

Slavek is suggesting that instead of chaning the function return type, we simply change the “return TQString()” instruction to a “return TQString::null” instruction.

I've thought about that, but will it be safe enough without memory leakage (will old value destruct correctly or not)?

I'm about this part.

Slavek is suggesting that instead of chaning the function return type, we simply change the “return TQString()” instruction to a “return TQString::null” instruction.

I've thought about that, but will it be safe enough without memory leakage (will old value destruct correctly or not)?

Yup, it would be safe. TQString::null is a static object so it will be there as long as the application is there. If you can reproduce the crash, it would be easy for you to verify whether the crash is gone even with the new idea from Slavek.

By default TQString creates an empty string with shared_null. I'm not sure the crash is the result of a local object. Really. But TQString::null is the right way.

I'm not sure the crash is the result of a local object. Really.

Crash is result of throwing a zero pointer to the codec->fromUnicode. Don't know why zero, possibly optimizator's work, but zero ptr or ptr to mess in stack - not much difference.

It even didn't work in the following code, placed to a main.cpp:

#include <tqtextcodec.h>
const TQString& foo(void) {
	return TQString();
}

<main> {
<...>
  TQString bar;

  bar = foo(); // <-- No source available for "TQString::operator=() at 0x7ffff6a5155d" 
}

As for TQString::null - i see that it was deprecated in Qt4, will you guys move the same way? Probably it will be more secure to not use it in the future.

I don't know what are the reasons to deprecate QString::null in Qt4, but for TQt3 there is currently no plan to remove it.

It seems advantageous that for cases like this, there is a TQString::null that does not need to be allocated again and again.

Crash is result of throwing a zero pointer to the codec->fromUnicode. Don't know why zero, possibly optimizator's work, but zero ptr or ptr to mess in stack - not much difference.

It even didn't work in the following code, placed to a main.cpp:
#include <tqtextcodec.h>
const TQString& foo(void) {

Here compiler creates an Object in automatic storage
(stack in Intel notation and stack frame for sparc)

return TQString();
Here the object is already destroyed, the reference points to an invalid data
}
{ <...> TQString bar;
bar = foo(); // <-- No source available for "TQString::operator=() at 0x7ffff6a5155d"

Here the argument of operator= is incorrect becase it points to the invalid data returned by foo(). However, at this point the storage can be reused by a signal or something else like that. In any case, access to the data pointed by the reference is impossible and you see SIGSEGV.
This is UB by the standard (undefined behaviour) and it completely depends of the compiler.

}


As for `TQString::null` - i see that it was deprecated in Qt4, will you guys move the same way? Probably it will be more secure to not use it in the future.

This is UB by the standard (undefined behaviour) and it completely depends of the compiler.

My embedded skill suggests me: if there an UB, then avoid it. That's why this PR was made for.

Ok, switched to TQString::null.

The reason of the crash is simple as Denis pointed out. The reference points to a TQString object in memory that is no longer valid. That area of memory is almost surely and immediately overwritten with some other values (being in the automatic storage 😉). When trying to use the referenced invalid TQString object, we could or could not see a crash depending on what values are there. If we are lucky the internal "d" pointer is invalid and we get SEGV. If we are unluckly, things may still work but we would 99.9% end up with incorrect or corrupted values.

In any case, the new PR is good, merged already!

@Mashiro welcome and thanks. Looking forward for more PR from you!!

ah yes, is it ok to close bug 1550 now?

ah yes, is it ok to close bug 1550 now?

Oh wait, no. Looks like i've messed up with links, sorry.

https://bugs.trinitydesktop.org/show_bug.cgi?id=2307 bug is fixed by this PR. 1550 is possibly related to the #7

Ok, thanks. Bug 2307 closed!