Bug 39537 - attachments' temporary files are deleted too soon
Summary: attachments' temporary files are deleted too soon
Status: RESOLVED FIXED
Alias: None
Product: kmail
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR wishlist with 35 votes (vote)
Target Milestone: ---
Assignee: kdepim bugs
URL:
Keywords:
: 53095 63742 67204 68001 92429 (view as bug list)
Depends on:
Blocks:
 
Reported: 2002-03-19 13:03 UTC by Will Stephenson
Modified: 2022-05-17 11:57 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
use hard links and KRun's tempfile flags to launch attachments (2.41 KB, patch)
2004-08-26 08:27 UTC, earlgrey
Details
use links and KRun's tempfile flag to launch attachments (2.32 KB, patch)
2004-08-31 10:01 UTC, earlgrey
Details
use hard links and KRun's tempfile flags to launch attachments (2.69 KB, patch)
2004-09-13 14:52 UTC, earlgrey
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Will Stephenson 2002-03-19 12:51:55 UTC
(*** This bug was imported into bugs.kde.org ***)

Package:           kmail
Version:           KDE 2.9.2 CVS/CVSup/Snapshot
Severity:          normal
Installed from:    Compiled sources
Compiler:          gcc 2.95.3
OS:                Linux
OS/Compiler notes: Not Specified

I notice that attachments are opened on click by creating a temporary file which is then passed as an argument to the appropriate viewer program.  

As soon as the current message changes (eg I spacebar down my inbox) however the temporary file is deleted so if the viewer has not yet launched and opened the file it complains that the file was not found.  

In effect it's a race condition between the viewer and my attention span!

Will

(Submitted via bugs.kde.org)
Comment 1 Hrishikesh Mehendale 2002-11-20 18:26:51 UTC
This is *really* noticable when using a slow-loading editor (like OpenOffice) 
when opening large Word DOC files ( PHBs never learn ;). Normally my policy 
would be to launch an attachment, and go ahead to the next unread (in my case 
with the + rather than Space).
Comment 2 Ingo Klöcker 2003-01-18 17:41:11 UTC
*** Bug 53095 has been marked as a duplicate of this bug. ***
Comment 3 Ingo Klöcker 2003-09-11 01:29:24 UTC
*** Bug 63742 has been marked as a duplicate of this bug. ***
Comment 4 Luís Pedro Coelho 2003-11-04 23:57:08 UTC
*** Bug 67204 has been marked as a duplicate of this bug. ***
Comment 5 Ingo Klöcker 2003-11-12 17:30:17 UTC
*** Bug 68001 has been marked as a duplicate of this bug. ***
Comment 6 Tim Wunder 2004-01-14 16:30:58 UTC
Why weren't the comments found in the duplicate bugs included as part of this bug?

FWIW, this behaviour still exists in KMail 1.5.4. Does anyone know if this issue will be resolved in the upcoming KDE 3.2 release?
Comment 7 Ingo Klöcker 2004-01-14 17:05:45 UTC
There's no need to duplicate the comments of the duplicates because the duplicates are linked to this bug report.

And, no, it won't be resolved in KDE 3.2. Otherwise we would have closed this bug report already.
Comment 8 earlgrey 2004-08-12 14:07:21 UTC
I want to fix this, using a list of kurls instead of QStringList mTempFiles [kmreaderwin] and toggling autodelete, but instead of the attach path being {...}/kmailxxxxx.y/filename, the attachment would be temporarily stored at {...}/kmail_xxx.$attID.$attname would that be acceptable?
That means the title in a word processor would be a bit ugly.
btw that's how ms does it.
Comment 9 earlgrey 2004-08-12 14:48:25 UTC
sorry, that should be KTempFile
Comment 10 earlgrey 2004-08-26 08:27:45 UTC
Created attachment 7293 [details]
use hard links and KRun's tempfile flags to launch attachments

Bug 39537, is about having to wait till the attachment is opened, before moving
to the next email.
This patch creates a hard link to the atm file, then uses the KRun's tempfile
flag to launch attachment.
Attachments launched will have names like ${atmName}_[xxxxxx].${extension}.

could someone review it?
Comment 11 David Faure 2004-08-26 11:42:40 UTC
You need to remove KMReaderWin:: from the .h file.

+  link(mAtmCurrentName.latin1(), linkName.latin1());
Should be QFile::encodeName()

+  return (linkName);
Looks like a method call :) We usually don't use () there :)

+ QFile( url.path() ).remove();
QFile::remove( url.path() ) might be a bit faster

Someone needs to check if kdelibs-3.2 had the "tempfile" flag. If not,
then the patch can't be backported to KDE_3_3_BRANCH. In any case
it's no problem for HEAD.

Comment 12 earlgrey 2004-08-31 10:01:55 UTC
Created attachment 7366 [details]
use links and KRun's tempfile flag to launch attachments

From the webcvs I see tempfile flags in KRun::run &&
KRun::displayOpenWithDialog in KDE_3_2_BRANCH.
I've fixed the issues raised by David.
Lets see, the patch is 32 lines of code, David found 4 problems, so we have an
error every 8 lines.
I'll add more comments next time, to achieve a lower error percentage ;-)
Comment 13 David Faure 2004-09-06 11:19:42 UTC
----------  Forwarded Message  ----------

Subject: Re: Fwd: [Bug 39537] attachments' temporary files are deleted too soon
Date: Monday 06 September 2004 05:18
From: George Staikos <staikos@kde.org>
To: David Faure <faure@kde.org>, security@kde.org


Actually it's quite a big race condition since the filename is known.  It's 
even easier to exploit than normal.  A script could just suck up cpu and 
constantly try to create files(links) with names that already exist.  It's 
probably sufficient to setAutoDelete(false);

On Wednesday 01 September 2004 07:46, you wrote:
> A little question for people with more security knowledge than me:
>
> Using the name of a just-deleted KTempFile is a small race condition,
> right? Any advice on how to do this better?
>
> +QString KMReaderWin::createTempAtmFile() const
> +{
> +  QFileInfo atmFileInfo(mAtmCurrentName);
> +  QString linkName;
> +
> +  KTempFile *linkFile = new KTempFile( locateLocal("tmp",
> atmFileInfo.fileName() +"_["), +                            "]."+
> atmFileInfo.extension() );
> +  linkFile->setAutoDelete(true);
> +  linkName = linkFile->name();
> +  delete linkFile;
> +
> +  link(QFile::encodeName(mAtmCurrentName), QFile::encodeName(linkName));
> +  //kdDebug(5004) << "link:" << mAtmCurrentName << " to " << 
> linkName.latin1() << endl; +  //kdDebug(5004) << "URL:" << url << " " <<
> mAtmCurrentName << endl; +  return linkName;
> +
> +}
>
>
> ----------  Forwarded Message  ----------
>
> Subject: [Bug 39537] attachments' temporary files are deleted too soon
> Date: Tuesday 31 August 2004 10:01
> From: earl grey <earlgreykde@bigpond.com>
> To: kmail-devel@kde.org
>
> ------- Additional Comments From earlgreykde bigpond com  2004-08-31 10:01
> ------- Created an attachment (id=7366)
>  --> (http://bugs.kde.org/attachment.cgi?id=7366&action=view)
> use links and KRun's tempfile flag to launch attachments
>
> -------------------------------------------------------

Comment 14 David Faure 2004-09-09 22:51:29 UTC
On Thursday 09 September 2004 21:25, Waldo Bastian wrote:
> On Wednesday 01 September 2004 13:46, David Faure wrote:
> > A little question for people with more security knowledge than me:
> >
> > Using the name of a just-deleted KTempFile is a small race condition,
> > right?
> 
> I don't think so, link will fail if the file already exists.... you may want 
> to check the return value though.

Yes (and try again with another tempfilename on EEXIST, right?)

> > Any advice on how to do this better? 
> 
> What are you trying to do? Is mAtmCurrentName located in "tmp" as well? 
> Otherwise you may not be able to link.

AFAIU this is about hardlinking to a short-lived tempfile in order to extend 
its lifetime for the duration of the launched application (that second
"tempfile" is then deleted by KRun, when the application exits).
I'll let the person who made the patch comment if I'm wrong, I'm just
the middle man here :)

Comment 15 Waldo Bastian 2004-09-10 10:36:40 UTC
> Yes (and try again with another tempfilename on EEXIST, right?)

I would just fail the whole operation in that case. It's very unlikely that you run into this situation because only the user is supposed to have write access to the directory in the first place.
Comment 16 earlgrey 2004-09-13 14:52:41 UTC
Created attachment 7506 [details]
use hard links and KRun's tempfile flags to launch attachments

Added a check to the link call and retry up to 10 times.
If it still can't create the link, then it uses the original file.
This all happens in the /tmp kstandarddir, which has perm 700, so security
shouldn't be a concern, and hardlinks should work.
Comment 17 David Faure 2004-09-14 16:42:29 UTC
CVS commit by faure: 

Apply fix for longstanding bug "attachments' temporary files are deleted too soon",
with thanks to "earl grey" for his patch (and patience :})
The KRun "delete tempfile after use" feature comes in handy for this (BTW I checked
and kdelibs-3.2 had it already).
Slightly altered the final patch to remove the loop as advised by Waldo.
CCMAIL: 39537-done@bugs.kde.org


  M +43 -5     kmreaderwin.cpp   1.777.2.2
  M +1 -0      kmreaderwin.h   1.197.2.1


--- kdepim/kmail/kmreaderwin.cpp  #1.777.2.1:1.777.2.2
@@ -1878,9 +1878,19 @@ void KMReaderWin::slotDoAtmOpen()
   }
 
-  KURL url;
-  url.setPath( mAtmCurrentName );
   KURL::List lst;
+  KURL url;
+  bool autoDelete = true;
+  QString fname = createAtmFileLink(); 
+
+  if ( fname == QString::null ) {
+    autoDelete = false;
+    fname = mAtmCurrentName;
+  }
+
+  url.setPath( fname );
   lst.append( url );
-  KRun::run( *mOffer, lst );
+  if ( (KRun::run( *mOffer, lst, autoDelete ) <= 0) && autoDelete ) {
+      QFile::remove(url.path());
+  }
 }
 
@@ -1893,7 +1903,17 @@ void KMReaderWin::slotAtmOpenWith()
     KURL::List lst;
     KURL url;
-    url.setPath(mAtmCurrentName);
+    bool autoDelete = true;
+    QString fname = createAtmFileLink(); 
+
+    if ( fname == QString::null ) {
+      autoDelete = false;
+      fname = mAtmCurrentName;
+    }
+
+    url.setPath( fname );
     lst.append(url);
-    KRun::displayOpenWithDialog(lst);
+    if ( (! KRun::displayOpenWithDialog(lst, autoDelete)) && autoDelete ) {
+      QFile::remove(url.path());
+    }
 }
 
@@ -2191,4 +2211,22 @@ void KMReaderWin::slotIMChat()
 
 //-----------------------------------------------------------------------------
+QString KMReaderWin::createAtmFileLink() const
+{
+  QFileInfo atmFileInfo(mAtmCurrentName);
+
+  KTempFile *linkFile = new KTempFile( locateLocal("tmp", atmFileInfo.fileName() +"_["),
+                          "]."+ atmFileInfo.extension() );
+
+  linkFile->setAutoDelete(true);
+  QString linkName = linkFile->name();
+  delete linkFile;
+
+  if ( link(QFile::encodeName(mAtmCurrentName), QFile::encodeName(linkName)) == 0 ) {
+    return linkName; // success
+  }
+  kdWarning() << "Couldn't link to " << mAtmCurrentName << endl;
+  return QString::null;
+}
+
 #include "kmreaderwin.moc"
 

--- kdepim/kmail/kmreaderwin.h  #1.197:1.197.2.1
@@ -388,4 +388,5 @@ private:
   void createActions( KActionCollection * ac );
   void saveSplitterSizes( KConfigBase & c ) const;
+  QString createAtmFileLink() const;
 
 private:


Comment 18 Carsten Burghardt 2004-11-06 17:09:37 UTC
*** Bug 92429 has been marked as a duplicate of this bug. ***
Comment 19 David Faure 2004-11-07 12:51:15 UTC
Fix reverted in KDE_3_3_BRANCH, since it breaks with apps that go into the background
like ark, kfmclient or korganizer. Proper fix will be done in HEAD tomorrow.

Comment 20 Arne Schmitz 2004-11-07 15:29:36 UTC
Has this already been fixed? Still got the problem in KDE 3.3.1.
Comment 21 S. Burmeister 2004-11-07 15:38:22 UTC
No, and if it was reverted in BRANCH, it will not be fixed until 3.4, I guess. This means some more months without this fix.
Comment 22 David Faure 2004-11-08 13:03:38 UTC
Yep. Meanwhile you just have to make sure to wait until the file is opened, before switching to another mail.
Not really critical once you know this is how to make it work :)

Comment 23 S. Burmeister 2004-11-08 18:35:33 UTC
> Yep. Meanwhile you just have to make sure to wait
> until the file is opened, before switching to another mail.

This is actually not always true. In case of files that are opened by konqueror, e.g. *.xhtml attachments you will have to make sure that konqueror is already open, otherwise it won't startup quick enough even if you did not switch to another mail.
Comment 24 David Faure 2004-11-08 19:18:59 UTC
> ------- Additional Comments From sven.burmeister gmx net  2004-11-08 18:35 -------
> > Yep. Meanwhile you just have to make sure to wait
> > until the file is opened, before switching to another mail.
> 
> This is actually not always true. In case of files that are opened by konqueror, e.g. *.xhtml attachments you will have to make sure that konqueror is already open, otherwise it won't startup quick enough even if you did not switch to another mail.

That's with the guilty patch, not without it. This is why I reverted it, in the branch.

Comment 25 Iñaki Baz Castillo 2005-02-10 02:05:13 UTC
Is this bug really fixed in KDE 3.3.2 ?
I use Debian Sid with KDE 3.3.2 and KMail 1.7.2, and the bug still occurs when changing to another mail when there is opening a attached file. The temporary file is deleted and the external program can't find it.
Why is this bug "FIXED"?
Comment 26 S. Burmeister 2005-02-10 09:58:00 UTC
> Is this bug really fixed in KDE 3.3.2 ?

I am not sure about 3.3.2, it never happened to me in 3.4 beta1 or 2, so I 
think it is fixed, yet not in 3_3_BRANCH.

Comment 27 Jonathan Marten 2006-10-29 15:23:23 UTC
*** Bug 133385 has been marked as a duplicate of this bug. ***
Comment 28 Jonathan Marten 2006-10-29 16:48:34 UTC
*** Bug 130709 has been marked as a duplicate of this bug. ***
Comment 29 alancio 2007-03-22 22:44:29 UTC
This bug is present in kde 3.5.6 (kmail 1.9.6).
kmail should wait() for the viewer application to exit before deleting the temp file.
Comment 30 Andres Mujica - SEAQ 2007-05-06 03:47:23 UTC
Hi. i'm linking this bug with ubuntu's launchpad corresponding bug

https://bugs.launchpad.net/kdepim/+bug/70981

I hope this can be addressed soon.

Thanks a lot for your help 
Comment 31 Halla Rempt 2022-04-11 09:39:33 UTC
Git commit e8319587df9862b0f6f5ea18114058dcf84194c4 by Halla Rempt.
Committed on 11/04/2022 at 09:38.
Pushed by rempt into branch 'rempt/work/bug_39537'.

Truncate the log file if it's bigger than 100mb

Maybe we should make a back-up, though?

M  +9    -1    libs/global/KisUsageLogger.cpp

https://invent.kde.org/graphics/krita/commit/e8319587df9862b0f6f5ea18114058dcf84194c4
Comment 32 Bug Janitor Service 2022-05-17 11:57:28 UTC
A possibly relevant merge request was started @ https://invent.kde.org/graphics/krita/-/merge_requests/1412