KWallet: Port QRegExp to QRegularExpression
ClosedPublic

Authored by ahmadsamir on Dec 24 2019, 9:23 AM.

Details

Summary

Port QRegExp::exactMatch() with QRegularExpression::anchoredPattern()
Port QRegExp::Wildcard with QRegularExpression::wildcardToRegularExpression()

The code compiles (there is only one unrelated unit test, it passes).

Diff Detail

Repository
R311 KWallet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ahmadsamir created this revision.Dec 24 2019, 9:23 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 24 2019, 9:23 AM
ahmadsamir requested review of this revision.Dec 24 2019, 9:23 AM
apol accepted this revision.Dec 24 2019, 11:50 AM
This revision is now accepted and ready to land.Dec 24 2019, 11:50 AM
This revision was automatically updated to reflect the committed changes.
blaze added a subscriber: blaze.Jan 15 2020, 1:55 PM

This changeset breaks the app, making it impossible to get the list of entries from outside. Tested against Falkon browser with KDE integration plugin. See my inline comments here

src/api/KWallet/kwallet.cpp
179

anchoredPattern() is excessive here, it creates a horror like that "\\A(?:\\A(?:[^/]*)\\z)\\z" with the stuff being anchored twice.
It has to be removed with no doubt at all.

180

The output of wildcardToRegularExpression() method is different from what was before and it breaks the app. It has to be replaced with something else.

src/runtime/kwalletd/backend/kwalletbackend.cc
533

Same here as well.

534

and here

ahmadsamir added inline comments.Jan 15 2020, 3:26 PM
src/api/KWallet/kwallet.cpp
179

The pattern has to be anchored if we want to replicate what QRegExp::exactMatch() did, with \A and \z or ^ and $.

If you have a test case where it anchoredPattern() anchors twice, file a bug report upstream (to be honest, I don't know whether this is the intended behaviour or not).

180

Any suggestions? QRegularExpression::wildcardToRegularExpression() is what the upstream docs offer:

Wildcard matching

There is no direct way to do wildcard matching in QRegularExpression. However, the wildcardToRegularExpression method is provided to translate glob patterns into a Perl-compatible regular expression that can be used for that purpose.

A test case of what is broken would be appreciated, to try and fix/debug the issue.

blaze added inline comments.Jan 15 2020, 3:49 PM
src/api/KWallet/kwallet.cpp
179

QRegularExpression::wildcardToRegularExpression() already returns an anchored output. What do you mean?

ahmadsamir added inline comments.Jan 15 2020, 4:12 PM
src/api/KWallet/kwallet.cpp
179

I didn't get what you meant before.

I've just tested and you're right; I didn't know that wildcardToRegularExpression() returned an anchored pattern, the docs didn't say anything about that (if they did, I must have missed it).

I'll be filing a diff to fix that (and I'll have review all my other QRegExp port diffs, the same issue will be in more places than just here in kwallet...).

Thanks for catching this issue.

blaze added a comment.Jan 15 2020, 4:32 PM

A test case of what is broken would be appreciated, to try and fix/debug the issue.

src/api/KWallet/kwallet.cpp
180

The transformation is targeting file path globbing, which means in particular that path separators receive special treatment.

I'm pretty sure the file path case is different from what we have here.

Also

In order to match one of the special characters, place it in square brackets (for example, "[?]")

Doesn't it mean you can't just use a bare raw wildcard?

So my suggestion is just to rollback to the previous solution, at least temporarily, until we don't have something better.

blaze added a comment.EditedJan 15 2020, 7:01 PM

https://github.com/KDE/falkon/blob/master/src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp#L187

QMap<QString, QByteArray> entries;
if (m_wallet->readEntryList("*", entries) != 0) {
    qWarning() << "KWalletPasswordBackend::initialize Cannot read entries!";
    return;
}

This is the problematic code. As you can see it uses "*" as a wildcard. So may be it is possible just to use a different wildcard and it could be solved IDK

https://github.com/KDE/falkon/blob/master/src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp#L187

QMap<QString, QByteArray> entries;
if (m_wallet->readEntryList("*", entries) != 0) {
    qWarning() << "KWalletPasswordBackend::initialize Cannot read entries!";
    return;
}

This is the problematic code. As you can see it uses "*" as a wildcard. So may be it is possible just to use a different wildcard and it could be solved IDK

OK, I'll see if I can make it work and also fix the double-anchored issue; in a new diff, otherwise, we could just revert this patch.

@apol, @aacid WDYT?

blaze added a comment.Jan 15 2020, 8:38 PM

otherwise, we could just revert this patch

The rest of the code is OK. The part that works funny is just the wildcard method, and since it's relatively new, I hope there could be a good workaround until the situation stabilizes

aacid added a comment.Jan 15 2020, 9:28 PM

https://github.com/KDE/falkon/blob/master/src/plugins/KDEFrameworksIntegration/kwalletpasswordbackend.cpp#L187

QMap<QString, QByteArray> entries;
if (m_wallet->readEntryList("*", entries) != 0) {
    qWarning() << "KWalletPasswordBackend::initialize Cannot read entries!";
    return;
}

This is the problematic code. As you can see it uses "*" as a wildcard. So may be it is possible just to use a different wildcard and it could be solved IDK

OK, I'll see if I can make it work and also fix the double-anchored issue; in a new diff, otherwise, we could just revert this patch.

@apol, @aacid WDYT?

I was going to be silent here, but since you asked my opinion is this, if you are not going to test the code you are changing, you should stop changing code. And by reading the comments it seems you aren't testing it, so either start testing it or stop doing blind changes.

src/api/KWallet/kwallet.cpp
180

The part about "[?]"; in the QRegularExpression documentation https://doc.qt.io/qt-5/qregularexpression.html#wildcardToRegularExpression "?" is used for wild card matching:

? Matches any single character. It is the same as . in full regexps.

IIUC, the comment about "[?]" means if wildcardToRegularExpression() is used and you want to match a literal "?" character you'll have to use square brackets "[?]", different issue.

otherwise, we could just revert this patch

The rest of the code is OK. The part that works funny is just the wildcard method, and since it's relatively new, I hope there could be a good workaround until the situation stabilizes

I still fail to see what's broken, I tested with the system kwallet (5.65 on tumbleweed) and I don't see any difference between it and a build from a git checkout; i.e. falkon doesn't show the saved passwords after restarting it. I've already submitted another diff to fix the doubly-anchored pattern issue.

Could you give me another test case? I want to see what's broken to try and fix it.

Thanks in advance.

blaze added a comment.Jan 16 2020, 5:20 PM

I still fail to see what's broken, I tested with the system kwallet (5.65 on tumbleweed) and I don't see any difference between it and a build from a git checkout; i.e. falkon doesn't show the saved passwords after restarting it. I've already submitted another diff to fix the doubly-anchored pattern issue.

falkon doesn't show the saved passwords

It's not normal behavior anyways. But have you forgot to restart kwalletd process while testing?

blaze added a comment.Jan 17 2020, 2:50 PM

I was debugging the code locally and here's what I found: simple wildcard like * is being replaced with cool stuff like \\A(?:[^/]*)\\z.
As you can see, this pattern excludes forward slashes, which may be a good thing for a file path, but it is exactly the source of the problem in this case.
So here's my debug code, which was tested by me and it works:

QRegularExpression re(QRegularExpression::wildcardToRegularExpression(key)
                       .replace("[^/]", "."));
qDebug() << " == PATTERN == " << re.pattern();