[FileProtocol] change statx stat_dev() to return makedev(major, minor)
ClosedPublic

Authored by ahmadsamir on Apr 1 2020, 9:18 AM.

Details

Summary

On Linux systems that have statx available, stat_dev() returned
stx_dev_major, which could be the same value for different partitions.
Change it to return makedev(stx_dev_major, stx_dev_minor) to get a unique
ID (meaning a unique UDSEntry::UDS_DEVICE_ID).

Add a unit test to check that UDS_DEVICE_ID is unique for each individual
partition.

Test Plan

$ stat /usr/bin/file | grep Device
Device: 804h/2052d Inode: 9168 Links: 1
$ stat ~/.bashrc | grep Device
Device: 803h/2051d Inode: 97 Links: 1

Run jobtest unit test and check the debug output from statDetailsWithInode.

Diff Detail

Repository
R241 KIO
Branch
l-statx (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24898
Build 24916: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 1 2020, 9:18 AM
ahmadsamir requested review of this revision.Apr 1 2020, 9:18 AM
meven added inline comments.Apr 1 2020, 10:28 AM
autotests/jobtest.cpp
1383

Please add a separate test dedicated for Inode like statWithInode() like statSymlink does for instance.
Here you change the purpose of this test : default stat behavior with stat(), to default + inode with statDetails.

1400

UDS_CREATION_TIME is statx specific only on Linux
Just make it clear in the comment.

1402

KIO::UDSEntry::UDS_DEVICE_ID and KIO::UDSEntry::UDS_INODE are not statx specific (it is supported for all UNIX systems)

src/ioslaves/file/file_unix.cpp
285

We should probably use makedev http://man7.org/linux/man-pages/man3/makedev.3.html in fact to ensure to match original stat behavior.

302

Did you compare with the no-statx case ?
We should try to have the same output.

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?

meven added a comment.Apr 1 2020, 12:30 PM

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.

ahmadsamir updated this revision to Diff 79044.Apr 1 2020, 1:43 PM
ahmadsamir marked 2 inline comments as done.
ahmadsamir edited the summary of this revision. (Show Details)

Address comments

ahmadsamir added inline comments.Apr 1 2020, 1:56 PM
autotests/jobtest.cpp
1383

Righto. Done.

1400

In the new unit test, I think using HAVE_STATX is self-explanatory (I'll leave JobTest::stat() alone in this diff to keep the commit atomic).

src/ioslaves/file/file_unix.cpp
285

I am not an expert on stat low-level system calls; but if the goal is to have a unique UDS_DEVICE_ID, we could leave it as uint32 which is what statx returns by default, it's system specific anyway, and a system will have statx or not...

302

This buf.st_dev is the decimal part in:
$ stat /usr/bin/file | grep Device
Device: 804h/2052d Inode: 9168 Links: 1
that would be "2052"

whereas with statx it returns buf.stx_dev_major which is an uint32_t.

bruns added a subscriber: bruns.Apr 1 2020, 4:36 PM
bruns added inline comments.
src/ioslaves/file/file_unix.cpp
285

* 100 is definitely wrong - maybe * 0x100, but ...

TLDR: use makedev, and change the return type to dev_t.

dev_t is defined as a 64bit type atleast on Linux, which matches the 32bit major/minor parts of buf.stx_dev_*. Traditionally, major/minor are used as low/high bytes of a 16 bit type, but these can easily exhausted on a larger system. So this is was makedev does:

#define __SYSMACROS_DEFINE_MAKEDEV(DECL_TEMPL)                  \
  __SYSMACROS_DECLARE_MAKEDEV (DECL_TEMPL)                      \
  {                                                             \
    __dev_t __dev;                                              \
    __dev  = (((__dev_t) (__major & 0x00000fffu)) <<  8);       \
    __dev |= (((__dev_t) (__major & 0xfffff000u)) << 32);       \
    __dev |= (((__dev_t) (__minor & 0x000000ffu)) <<  0);       \
    __dev |= (((__dev_t) (__minor & 0xffffff00u)) << 12);       \
    return __dev;                                               \
  }
bruns requested changes to this revision.Apr 1 2020, 4:36 PM
This revision now requires changes to proceed.Apr 1 2020, 4:36 PM
ahmadsamir updated this revision to Diff 79071.Apr 1 2020, 5:31 PM
ahmadsamir retitled this revision from [FileProtocol] change statx stat_dev() to use device major + minor to [FileProtocol] change statx stat_dev() to return makedev(major, minor).
ahmadsamir edited the summary of this revision. (Show Details)
ahmadsamir edited the test plan for this revision. (Show Details)
ahmadsamir removed a subscriber: bruns.

Use makedev()

ahmadsamir marked 3 inline comments as done.Apr 1 2020, 5:32 PM
ahmadsamir added inline comments.
src/ioslaves/file/file_unix.cpp
302

With makedev() now I get the same numbers.

meven added a comment.Apr 2 2020, 11:23 AM

Just a few nitpicks in test and this is good IMHO

autotests/jobtest.cpp
1516

Can remove

1530

If you want to avoid doing this you can remove fields asked from statDetails KIO::StatBasic | KIO::StatUser | KIO::StatInode for instance.
And have a more focused test.

1566

You can remove this now ;)

1568

You can have an else with just QCOMPARE(device, otherDevice); and move most of the code inside of if (!otherTmpDirIsOnSamePartition()) outside of the if branch.

ahmadsamir updated this revision to Diff 79133.Apr 2 2020, 12:23 PM
ahmadsamir marked an inline comment as done.
  • Less stat details, in that unit test we're interested in Inode stuff
  • In the test, return early if we can't find two separate partitions
ahmadsamir marked 4 inline comments as done.Apr 2 2020, 12:25 PM
ahmadsamir added inline comments.
autotests/jobtest.cpp
1530

Good point, I slimmed it down to just KIO::StatInode. (Maybe that's too thin?).

1568

I moved the condition to the top, if we can't find two partitions on the system, bail out early.

meven requested changes to this revision.Apr 4 2020, 10:15 AM

I just want to keep as much checks as possible

autotests/jobtest.cpp
1524

add QCOMPARE(entry.count(), 2);

1535

add QCOMPARE(otherEntry.count(), 2);

This revision now requires changes to proceed.Apr 4 2020, 10:15 AM
bruns added inline comments.Apr 4 2020, 11:09 AM
autotests/jobtest.cpp
1524

Its 'statDetailsWithInode' not 'statDetailsInodeOnly'

bruns added inline comments.Apr 4 2020, 11:22 AM
autotests/jobtest.cpp
1510

This should only apply to the QVERIFY(device != otherDevice);

1524

i.e. the test should be renamed now.

ahmadsamir updated this revision to Diff 79315.Apr 4 2020, 3:19 PM
ahmadsamir marked 2 inline comments as done.
  • Change unit test name
  • More QCOMPARE()
ahmadsamir added inline comments.Apr 4 2020, 5:34 PM
autotests/jobtest.cpp
1510

Why? The goal is to test if the UDS_DEVICE_ID is unique for individual partitions, which won't work unless there are two separate partitions.

bruns added inline comments.Apr 4 2020, 6:20 PM
autotests/jobtest.cpp
1510

The first goal should be to test if it works at all, i.e. if there is an UDS_DEVICE_ID/INODE at all.

ahmadsamir updated this revision to Diff 79339.Apr 4 2020, 7:06 PM

Skip the smallest part of the unit test

ahmadsamir marked an inline comment as done.Apr 4 2020, 7:07 PM
ahmadsamir added inline comments.
autotests/jobtest.cpp
1510

OK, that makes sense. Done.

meven accepted this revision.Apr 4 2020, 7:55 PM

Nice, plus a nice test added.
Give some time to others to have a final say before merging.

bruns added a comment.Apr 5 2020, 7:41 AM

looks good to me otherwise

autotests/jobtest.cpp
1510

nitpick - the test name now again is wrong ;-)

1537

slight rewording

// In case the two tmp dirs are on the same partition (e.g. on the CI), the
// device IDs should  match, otherwise they should be different

and then

} else {
   QCOMPARE(device, otherDevice);
}
ahmadsamir updated this revision to Diff 79410.Apr 5 2020, 1:24 PM
ahmadsamir marked an inline comment as done.

Address comments

bruns added inline comments.Apr 5 2020, 1:37 PM
autotests/jobtest.cpp
1540

QCOMPARE(device, otherDevice);

autotests/jobtest.h
88

wrong name

ahmadsamir updated this revision to Diff 79411.Apr 5 2020, 2:02 PM

Fix test name in header
Add one more QCOMPARE()

ahmadsamir marked 2 inline comments as done.Apr 5 2020, 2:03 PM
ahmadsamir added inline comments.
autotests/jobtest.h
88

Of course, the one time I forgot to build before using 'arc diff' in a long time...

meven accepted this revision.Apr 5 2020, 3:18 PM
ahmadsamir updated this revision to Diff 79567.Apr 7 2020, 1:21 PM
ahmadsamir marked an inline comment as done.

Rebase

This revision was not accepted when it landed; it landed in state Needs Review.Apr 7 2020, 1:22 PM
This revision was automatically updated to reflect the committed changes.
meven added a comment.Apr 9 2020, 12:32 PM

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?

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 & :-)

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.

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.

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()"...