Ticket #13315: 0001-Drop-internal-use-of-QUrl-in-FileTransfer.patch

File 0001-Drop-internal-use-of-QUrl-in-FileTransfer.patch, 13.6 KB (added by ijc, 22 months ago)

[PATCH 1/2] Drop internal use of QUrl in FileTransfer?

  • mythtv/libs/libmythprotoserver/requesthandler/fileserverhandler.cpp

    From a34862355ed83638ab97dc53d94e3d5f82e1a0a3 Mon Sep 17 00:00:00 2001
    From: Ian Campbell <ijc@hellion.org.uk>
    Date: Sat, 1 Sep 2018 08:34:10 +0100
    Subject: [PATCH 1/2] Drop internal use of QUrl in FileTransfer
    
    Previously it was not possible to access files with certain characters in them
    remotely (such as music tracks with quotes in the title) because the characters
    are URL %-escaped, e.g.:
    
        ProcessRequest storagegroup.cpp:608 (FindFile) SG(Music): FindFile: Searching for '/Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(Music): FindFileDir: Checking '/storage/music' for '/storage/music//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(Default): FindFileDir: Checking '/var/lib/mythtv/recordings' for '/var/lib/mythtv/recordings//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/lib/mythtv/.mythtv/Banners' for '/var/lib/mythtv/.mythtv/Banners//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/lib/mythtv/.mythtv/Coverart' for '/var/lib/mythtv/.mythtv/Coverart//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/backups/mythtv' for '/var/backups/mythtv//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/lib/mythtv/recordings' for '/var/lib/mythtv/recordings//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/lib/mythtv/.mythtv/Fanart' for '/var/lib/mythtv/.mythtv/Fanart//Enslaved - Isa/01-Intro: %22Gree  Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/storage/music' for '/storage/music//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/lib/mythtv/.mythtv/MusicArt' for '/var/lib/mythtv/.mythtv/MusicArt//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/storage/media' for '/storage/media//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/lib/mythtv/.mythtv/Screenshots' for '/var/lib/mythtv/.mythtv/Screenshots//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/lib/mythtv/.mythtv/Trailers' for '/var/lib/mythtv/.mythtv/Trailers//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:639 (FindFileDir) SG(): FindFileDir: Checking '/var/lib/mythvideo' for '/var/lib/mythvideo//Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'
        ProcessRequest storagegroup.cpp:622 (FindFile) SG(Music): FindFile: Unable to find '/Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'!
        ProcessRequest mainserver.cpp:7968 (LocalFilePath) MainServer: ERROR: LocalFilePath unable to find local path for '/Enslaved - Isa/01-Intro: %22Green Reflection%22.flac'.
    
    This is because the actual file on disk is not %-encoded, the actual filename
    is `01-Intro: "Green Reflection".flac`. If the filename had been %-encoded,
    then the resulting %'s would, I believe, have themselves been encoded as %25 by
    this point.
    
    Using QUrl here is adding nothing, it simply %encodes everything (requiring us
    to request decode manually) and drops any apparent query or fragment (the bits
    after `?` or `#`) requiring us to put them back, which we currently do
    incorrectly by not handling both in all cases.
    
    The thing we have in our hand is not really a URL, it's just a string path
    (which might contain URL special characters, causing the above undesirable
    behaviours), just use a QString for it.
    
    There seems to be two very similar copies of this code (libmythprotoserver and
    in mythbackend/mainserver directly) so I have adjusted both. It's not entirely
    clear to me where they are each used from, but I have tested (on a backport to
    fixes/29):
    
    - recording playback (both preexisting recordings and live TV), the filenames
      here are autogenerated and are not expected to contain any quotable
      characters AFAIK.
    - mythvideo (filenames with and without quote characters).
    - mythmusic (filenames with and without quote characters).
    - old and new gallery plugins (only filenames without quotes).
    
    Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
    ---
     .../requesthandler/fileserverhandler.cpp      | 27 +++++++-------
     .../requesthandler/fileserverhandler.h        |  2 +-
     mythtv/programs/mythbackend/mainserver.cpp    | 37 ++++++++-----------
     mythtv/programs/mythbackend/mainserver.h      |  2 +-
     4 files changed, 30 insertions(+), 38 deletions(-)
    
    diff --git a/mythtv/libs/libmythprotoserver/requesthandler/fileserverhandler.cpp b/mythtv/libs/libmythprotoserver/requesthandler/fileserverhandler.cpp
    index 08fbe5d4ad..c71fdc3205 100644
    a b void FileServerHandler::connectionClosed(MythSocket *socket) 
    5959    }
    6060}
    6161
    62 QString FileServerHandler::LocalFilePath(const QUrl &url,
     62QString FileServerHandler::LocalFilePath(const QString &path,
    6363                                           const QString &wantgroup)
    6464{
    65     QString lpath = url.path();
     65    QString lpath = QString(path);
    6666
    6767    if (lpath.section('/', -2, -2) == "channels")
    6868    {
    QString FileServerHandler::LocalFilePath(const QUrl &url, 
    121121            if (!wantgroup.isEmpty())
    122122            {
    123123                sgroup.Init(wantgroup);
    124                 lpath = url.toString();
     124                lpath = QString(path);
    125125            }
    126126            else
    127127            {
    QString FileServerHandler::LocalFilePath(const QUrl &url, 
    135135                LOG(VB_FILE, LOG_INFO,
    136136                        QString("LocalFilePath(%1 '%2'), found through "
    137137                                "exhaustive search at '%3'")
    138                             .arg(url.toString()).arg(opath).arg(lpath));
     138                            .arg(path).arg(opath).arg(lpath));
    139139            }
    140140            else
    141141            {
    bool FileServerHandler::HandleAnnounce(MythSocket *socket, 
    228228    }
    229229
    230230    QStringList::const_iterator it = slist.begin();
    231     QUrl qurl           = *(++it);
     231    QString path        = *(++it);
    232232    QString wantgroup   = *(++it);
    233233
    234234    QStringList checkfiles;
    bool FileServerHandler::HandleAnnounce(MythSocket *socket, 
    258258            return true;
    259259        }
    260260
    261         QString basename = qurl.path();
    262         if (basename.isEmpty())
     261        if (path.isEmpty())
    263262        {
    264263            LOG(VB_GENERAL, LOG_ERR, QString("FileTransfer write "
    265                     "filename is empty in url '%1'.")
    266                     .arg(qurl.toString()));
     264                    "filename is empty in path '%1'.")
     265                    .arg(path));
    267266
    268267            slist << "ERROR" << "filetransfer_filename_empty";
    269268            socket->WriteStringList(slist);
    270269            return true;
    271270        }
    272271
    273         if ((basename.contains("/../")) ||
    274             (basename.startsWith("../")))
     272        if ((path.contains("/../")) ||
     273            (path.startsWith("../")))
    275274        {
    276275            LOG(VB_GENERAL, LOG_ERR, QString("FileTransfer write "
    277276                    "filename '%1' does not pass sanity checks.")
    278                     .arg(basename));
     277                    .arg(path));
    279278
    280279            slist << "ERROR" << "filetransfer_filename_dangerous";
    281280            socket->WriteStringList(slist);
    282281            return true;
    283282        }
    284283
    285         filename = dir + "/" + basename;
     284        filename = dir + "/" + path;
    286285    }
    287286    else
    288         filename = LocalFilePath(qurl, wantgroup);
     287        filename = LocalFilePath(path, wantgroup);
    289288
    290289    QFileInfo finfo(filename);
    291290    if (finfo.isDir())
  • mythtv/libs/libmythprotoserver/requesthandler/fileserverhandler.h

    diff --git a/mythtv/libs/libmythprotoserver/requesthandler/fileserverhandler.h b/mythtv/libs/libmythprotoserver/requesthandler/fileserverhandler.h
    index 8210e1c1fe..a98a4797aa 100644
    a b class PROTOSERVER_PUBLIC FileServerHandler : public SocketRequestHandler 
    4949                                 QStringList &slist);
    5050    bool HandleDownloadFile(SocketHandler *socket, QStringList &slist);
    5151
    52     QString LocalFilePath(const QUrl &url, const QString &wantgroup);
     52    QString LocalFilePath(const QString &path, const QString &wantgroup);
    5353    void RunDeleteThread(void);
    5454
    5555    QMap<int, FileTransfer*>        m_ftMap;
  • mythtv/programs/mythbackend/mainserver.cpp

    diff --git a/mythtv/programs/mythbackend/mainserver.cpp b/mythtv/programs/mythbackend/mainserver.cpp
    index ce9ace4367..6f2a09e5fa 100644
    a b using namespace std; 
    3434#include <QWriteLocker>
    3535#include <QRegExp>
    3636#include <QEvent>
    37 #include <QUrl>
    3837#include <QTcpServer>
    3938#include <QTimer>
    4039#include <QNetworkInterface>
    void MainServer::HandleAnnounce(QStringList &slist, QStringList commands, 
    19091908        LOG(VB_NETWORK, LOG_INFO, LOC +
    19101909            QString("adding: %1 as a remote file transfer") .arg(commands[2]));
    19111910        QStringList::const_iterator it = slist.begin();
    1912         QUrl qurl = *(++it);
     1911        QString path = *(++it);
    19131912        QString wantgroup = *(++it);
    19141913        QString filename;
    19151914        QStringList checkfiles;
     1915
    19161916        for (++it; it != slist.end(); ++it)
    19171917            checkfiles += *it;
    19181918
    void MainServer::HandleAnnounce(QStringList &slist, QStringList commands, 
    19451945                return;
    19461946            }
    19471947
    1948             QString basename = qurl.path();
    1949             if (qurl.hasFragment())
    1950                 basename += "#" + qurl.fragment();
    1951 
    1952             if (basename.isEmpty())
     1948            if (path.isEmpty())
    19531949            {
    19541950                LOG(VB_GENERAL, LOG_ERR, LOC +
    1955                     QString("FileTransfer write filename is empty in url '%1'.")
    1956                         .arg(qurl.toString()));
     1951                    QString("FileTransfer write filename is empty in path '%1'.")
     1952                        .arg(path));
    19571953                errlist << "filetransfer_filename_empty";
    19581954                socket->WriteStringList(errlist);
    19591955                return;
    19601956            }
    19611957
    1962             if ((basename.contains("/../")) ||
    1963                 (basename.startsWith("../")))
     1958            if ((path.contains("/../")) ||
     1959                (path.startsWith("../")))
    19641960            {
    19651961                LOG(VB_GENERAL, LOG_ERR, LOC +
    19661962                    QString("FileTransfer write filename '%1' does not pass "
    1967                             "sanity checks.") .arg(basename));
     1963                            "sanity checks.") .arg(path));
    19681964                errlist << "filetransfer_filename_dangerous";
    19691965                socket->WriteStringList(errlist);
    19701966                return;
    19711967            }
    19721968
    1973             filename = dir + "/" + basename;
     1969            filename = dir + "/" + path;
    19741970        }
    19751971        else
    1976             filename = LocalFilePath(qurl, wantgroup);
     1972            filename = LocalFilePath(path, wantgroup);
    19771973
    19781974        if (filename.isEmpty())
    19791975        {
    void MainServer::SetExitCode(int exitCode, bool closeApplication) 
    81408136        QCoreApplication::exit(m_exitCode);
    81418137}
    81428138
    8143 QString MainServer::LocalFilePath(const QUrl &url, const QString &wantgroup)
     8139QString MainServer::LocalFilePath(const QString &path, const QString &wantgroup)
    81448140{
    8145     QString lpath = url.path();
    8146 
    8147     if (url.hasFragment())
    8148         lpath += "#" + url.fragment();
     8141    QString lpath = QString(path);
    81498142
    81508143    if (lpath.section('/', -2, -2) == "channels")
    81518144    {
    QString MainServer::LocalFilePath(const QUrl &url, const QString &wantgroup) 
    82048197            if (!wantgroup.isEmpty())
    82058198            {
    82068199                sgroup.Init(wantgroup);
    8207                 lpath = url.toString();
     8200                lpath = QString(path);
    82088201            }
    82098202            else
    82108203            {
    QString MainServer::LocalFilePath(const QUrl &url, const QString &wantgroup) 
    82188211                LOG(VB_FILE, LOG_INFO, LOC +
    82198212                    QString("LocalFilePath(%1 '%2'), found file through "
    82208213                            "exhaustive search at '%3'")
    8221                         .arg(url.toString()).arg(opath).arg(lpath));
     8214                        .arg(path).arg(opath).arg(lpath));
    82228215            }
    82238216            else
    82248217            {
    82258218                LOG(VB_GENERAL, LOG_ERR, LOC + QString("ERROR: LocalFilePath "
    8226                     "unable to find local path for '%1'.") .arg(url.toString()));
     8219                    "unable to find local path for '%1'.") .arg(path));
    82278220                lpath = "";
    82288221            }
    82298222
  • mythtv/programs/mythbackend/mainserver.h

    diff --git a/mythtv/programs/mythbackend/mainserver.h b/mythtv/programs/mythbackend/mainserver.h
    index be3e4b5d21..52773ee4a9 100644
    a b class MainServer : public QObject, public MythSocketCBs 
    276276    FileTransfer *GetFileTransferByID(int id);
    277277    FileTransfer *GetFileTransferBySock(MythSocket *socket);
    278278
    279     QString LocalFilePath(const QUrl &url, const QString &wantgroup);
     279    QString LocalFilePath(const QString &path, const QString &wantgroup);
    280280
    281281    int GetfsID(QList<FileSystemInfo>::iterator fsInfo);
    282282