I'll address the comments, but one point which I couldn't figure out is that HAVE_STATX isn't available for the unit tests, any examples on how to make it work there too?

In D28478#639294, @ahmadsamir wrote:

I'll address the comments, but one point which I couldn't figure out is that HAVE_STATX isn't available for the unit tests, any examples on how to make it work there too?

I tried to do it as well, but it seems quite complicated to share it with the main code base and I couldn't find a way.
The variable is added by src/ioslaves/file/ConfigureChecks.cmake and src/ioslaves/file/config-kioslave-file.h.cmake in the first place.

Just a few nitpicks in test and this is good IMHO

looks good to me otherwise

The test is not stable (I reproduce locally)

https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.14/34/testReport/junit/projectroot/autotests/kiocore_jobtest/

==7022==ERROR: AddressSanitizer: heap-use-after-free on address 0x610000099be8 at pc 0x7f09559df9ed bp 0x7fff81ec0fe0 sp 0x7fff81ec0fd8
READ of size 8 at 0x610000099be8 thread T0
    #0 0x7f09559df9ec in QSharedDataPointer<KIO::UDSEntryPrivate>::operator->() const /usr/include/qt5/QtCore/qshareddata.h:82
    #1 0x7f09559d499a in KIO::UDSEntry::numberValue(unsigned int, long long) const /home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.14/src/core/udsentry.cpp:368
    #2 0x455940 in JobTest::statWithInode() /home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.14/autotests/jobtest.cpp:1534

Replacing line 1517

const KIO::UDSEntry &entry = job->statResult();

by

const KIO::UDSEntry entry = job->statResult();

Fixes the issue, but I don't exactly see why.

hmm... first this is a copy-paste "error" on my part, I personally never use & when the RHS is a temporary (I don't see the point).

Anyway; looking at statResult():
const UDSEntry &StatJob::statResult() const
{

return d_func()->m_statResult;

}

it is returning a const &, and the docs say:

Call this in the slot connected to result, and only after making sure no error happened.

here we used job->exec(), it may finish and get deleted; KJob docs say:

KJob and its subclasses are meant to be used in a fire-and-forget way. Jobs will delete themselves when they finish using deleteLater() (although this behaviour can be changed), so a job instance will disappear after the next event loop run.

The test never failed before for me; but it seems I can trigger a SIGSEGV by running the test many times in a row; it's my "educated guess", sort of confirmed by using job->setAutoDelete(false), that the job may finish and get deleted by the time we call entry.numberValue() on line 1534.

So, to use job->statResult() to initialize a var &, that has to happen from a slot connected to result(); but if we call job->exec(), then later on we want to use the UDSEntry then we can't use var &, but rather take a copy.

And maybe statResult() shouldn't return a reference (or a const reference for that matter, because if the caller uses 'entry = job->statResult()'
then statResult() returning a const & doesn't make any difference if it's copied...).

@dfaure, WDYT?

In D28478#645019, @ahmadsamir wrote:

hmm... first this is a copy-paste "error" on my part, I personally never use & when the RHS is a temporary (I don't see the point).

The point is to avoid a copy ;-)

If the RHS is a value, this extends its lifetime.
But if it's a reference itself, then indeed we depend on its lifetime....

the job may finish and get deleted by the time we call entry.numberValue() on line 1534.

That's not a "guess", it's confirmed by what ASAN tells us in its second backtrace.

And maybe statResult() shouldn't return a reference (or a const reference for that matter, because if the caller uses 'entry = job->statResult()'
then statResult() returning a const & doesn't make any difference if it's copied...).

Well you get two copies then, one at "return" and one at '=' (technically that's an assignment, unless entry is actually declared on the same line).

Anyhow the fix is clear, remove the & :-)

In D28478#645682, @dfaure wrote:

In D28478#645019, @ahmadsamir wrote:

hmm... first this is a copy-paste "error" on my part, I personally never use & when the RHS is a temporary (I don't see the point).

The point is to avoid a copy ;-)

If the RHS is a value, this extends its lifetime.
But if it's a reference itself, then indeed we depend on its lifetime....

the job may finish and get deleted by the time we call entry.numberValue() on line 1534.

That's not a "guess", it's confirmed by what ASAN tells us in its second backtrace.

And maybe statResult() shouldn't return a reference (or a const reference for that matter, because if the caller uses 'entry = job->statResult()'
then statResult() returning a const & doesn't make any difference if it's copied...).

Well you get two copies then, one at "return" and one at '=' (technically that's an assignment, unless entry is actually declared on the same line).

I meant something like:
const int i = foo();
the return from foo() is copied into i and then it's gone.
const int &i = foo();
the return from foo() will be stored in a temporary, and it will stay around until i goes out of scope. So, there's only one "copy" of the return from foo() around, I'd rather take the one stored in i (https://herbsutter.com/2013/05/13/gotw-2-solution-temporary-objects/); I know that's nitpicking, but anyway. So mainly when it's an initialisation rather than assignment. :)

Anyhow the fix is clear, remove the & :-)

OK, will do.

In D28478#645702, @ahmadsamir wrote:

I meant something like:
const int i = foo();

int is a very bad example for this reasoning. It's cheaper to copy an int than to use a reference. That's not the case for a non-POD type like UDSEntry.

the return from foo() is copied into i and then it's gone.
const int &i = foo();
the return from foo() will be stored in a temporary, and it will stay around until i goes out of scope.

Yes.

So, there's only one "copy" of the return from foo() around, I'd rather take the one stored in i

Yes. The other copy I was referring to is the one that happens *inside* the implementation of foo(), if it doesn't return a const ref and you do "return m_someMember;"

Some people optimize this by using a local const ref, some people return a const ref from foo(). Either one works, but doing both is dangerous, like here.
In general I'd say the best way is to return a value. Like Qt does everywhere.
Here I'm a bit unsure (about changing the return type in KF6, can't be done before anyway) because outside unittests we're not supposed to use exec() so there's no risk.

In D28478#645717, @dfaure wrote:

In D28478#645702, @ahmadsamir wrote:

I meant something like:
const int i = foo();

int is a very bad example for this reasoning. It's cheaper to copy an int than to use a reference. That's not the case for a non-POD type like UDSEntry.

Yep.

the return from foo() is copied into i and then it's gone.
const int &i = foo();
the return from foo() will be stored in a temporary, and it will stay around until i goes out of scope.

Yes.

So, there's only one "copy" of the return from foo() around, I'd rather take the one stored in i

Yes. The other copy I was referring to is the one that happens *inside* the implementation of foo(), if it doesn't return a const ref and you do "return m_someMember;"

Some people optimize this by using a local const ref, some people return a const ref from foo(). Either one works, but doing both is dangerous, like here.

hmm... so one needs to know whether foo() returns a value or a const &; if I don't know for sure, then use a non-ref LHS to be on the safe side.

In general I'd say the best way is to return a value. Like Qt does everywhere.
Here I'm a bit unsure (about changing the return type in KF6, can't be done before anyway) because outside unittests we're not supposed to use exec() so there's no risk.

And the docs there say explicitly "call this from a slot connected to result()"...