Bug 79813 - [testcase] If a POST request leads to a downloadable file, and if that name already exists locally, a second GET (without form data!) is issued to get the file
Summary: [testcase] If a POST request leads to a downloadable file, and if that name a...
Status: RESOLVED FIXED
Alias: None
Product: konqueror
Classification: Applications
Component: khtml (show other bugs)
Version: 4.0.1
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Konqueror Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-04-17 14:34 UTC by Alain Knaff
Modified: 2011-12-05 19:00 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.6.0


Attachments
Patch for file io-slave (1.26 KB, patch)
2004-08-01 17:21 UTC, Waldo Bastian
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alain Knaff 2004-04-17 14:34:47 UTC
Version:           3.2.1 (using KDE 3.2.1, SuSE)
Compiler:          gcc version 3.3.1 (SuSE Linux)
OS:          Linux (i686) release 2.6.4

I have a website for constructing udpcast images online:
http://udpcast.linux.lu/cast-o-matic/udpflop.img

After two configuration screens, you click on a button to download the image. A click on this button submits all parameters to the server via a POST request, and the server sends back the image (with MIME-type application/octet-stream) in response.

This works fine if a similarly named file does not already exist locally.

However, if a similarly named file does exist, konqueror pops up an Overwrite/SuggestNewName/Cancel box, which is fine.

If the user then clicks "overwrite", the request is resubmitted, as a GET, and without the form data.

Wouldn't the correct behavior to be:
 1. Not submit a new request, but save the data which it already got from the server? (Just like what happens if there is no name clash)
 2. Or, if resubmitting is unavoidable, at least resubmit it the same way it was the first time (POST, with form data)

N.B. The same thing works fine in Mozilla, even when overwriting a file.
Comment 1 Alain Knaff 2004-04-17 14:36:31 UTC
Oops, the URL I supplied is the URL for the second form.

The real one (first form) is: http://udpcast.linux.lu/cast-o-matic/
Comment 2 Kevin Puetz 2004-07-18 09:33:02 UTC
I just saw this same thing happen to me with the squirrelmail 'archive folder' plugin. After much puzzlement about why it didn't work in konqi of course. So it's real, I'll try to put up a simple form/cgi that exhibit the problem to make it easier to debug...
Comment 3 Kevin Puetz 2004-07-18 10:19:16 UTC
http://puetzk.org/test/kio_http/ has a small testcase
Comment 4 Kevin Puetz 2004-07-18 10:20:07 UTC
Bug #68541 also looks related?
Comment 5 Kevin Puetz 2004-07-18 21:32:37 UTC
I can still produce this in 3.2.3, but according to uga it's no longer reproduceable in HEAD (from approx 7/9/04), so it's probably been fixed. I don't know what work has been done in this code recently, but is there any chance it's a small enough fix for backporting to the branch?
Comment 6 Dawit Alemayehu 2004-07-21 09:01:14 UTC
No, the original bug still stands in HEAD. It only happens if the "overwrite" scenario described in the report occurs. Anyways, I will see if I can track down the bug. It is somewhere in KIO...
Comment 7 Kevin Puetz 2004-07-24 06:35:52 UTC
ok, in 3.2 I'm seeing even in the non-overwrite case, which uga said was no longer reproduceable (for him, I don't have a HEAD build at the moment). So maybe they weren't related after all...
Comment 8 Kevin Puetz 2004-07-28 04:48:52 UTC
Ok, who knows what uga was testing with, but I can also still reproduce this in 3.3beta2. So it's definitely still around.
Comment 9 Waldo Bastian 2004-08-01 16:55:07 UTC
Following log illustrates the problem:

kio (KIOJob): KIO::copy src=http://localhost/br79813.bin dest=file:/tmp/br79813.bin
kio (KIOJob): kio_uiserver registered
kio (KIOJob): Created job 0x889a258 with progress info -- m_progressId=101
kio (KIOJob): stat file:/tmp/br79813.bin
kio (KIOJob): CopyJob:stating the dest file:/tmp/br79813.bin
kio (KIOJob): addSubjob(0x87c25d8) this = 0x889a258
kio (KIOJob): StatJob::slotStatEntry
kio (KIOJob): CopyJob::slotResult() state=0
kio (KIOJob): CopyJob::slotResultStating
kio (KIOJob): CopyJob::slotResultStating dest is dir:false
kio (KIOJob): stat http://localhost/br79813.bin
kio (KIOJob): KIO::stat on http://localhost/br79813.bin
kio (KIOJob): addSubjob(0x826acd8) this = 0x889a258
kio (KIOJob): StatJob::slotStatEntry
kio (KIOJob): CopyJob::slotResult() state=0
kio (KIOJob): CopyJob::slotResultStating
kio (KIOJob): CopyJob::slotEntries 'br79813.bin'
kio (KIOJob):  uSource=http://localhost/br79813.bin uDest(1)=file:/tmp/br79813.bin
kio (KIOJob):  uDest(2)=file:/tmp/br79813.bin
kio (KIOJob):  http://localhost/br79813.bin -> file:/tmp/br79813.bin
kio (KIOJob):  Source is a file (or a symlink), or we are linking -> no recursive listing
kio (KIOJob): CopyJob::copyNextFile()
kio (KIOJob): copying /tmp/br79813.bin
kio (KIOJob): FileCopyJob::FileCopyJob()
kio (KIOJob): CopyJob::copyNextFile : Copying http://localhost/br79813.bin to file:/tmp/br79813.bin
kio (KIOJob): addSubjob(0x87c22f8) this = 0x889a258
kio (KIOJob): FileCopyJob::startDataPump()
kio (KIOJob): FileCopyJob: m_putJob = 0x82b39d8 m_dest=file:/tmp/br79813.bin
kio (KIOJob): addSubjob(0x82b39d8) this = 0x87c22f8
kio (KIOJob): FileCopyJob::slotCanResume from PUT job. offset=0
kio (KIOJob): FileCopyJob: m_getJob = 0x826c398
kio (KIOJob): addSubjob(0x826c398) this = 0x87c22f8
kio (Scheduler): Resume metadata is ''
kio (Scheduler): HOLD: Reusing held slave for http://localhost/br79813.bin
kio (KIOJob): Single file -> updating totalsize to 0
kio (KIOJob): FileCopyJob::slotData
kio (KIOJob):  data size : 8192
kio (KIOJob): FileCopyJob::slotDataReq
kio (KIOJob): FileCopyJob::slotData
kio (KIOJob):  data size : 8192
kio (KIOJob): error 12 /tmp/br79813.bin
kio (KIOJob): TransferJob::slotFinished(0x82b39d8, file:/tmp/br79813.bin)
kio (KIOJob): FileCopyJob this=0x87c22f8 ::slotResult(0x82b39d8)
kio (Scheduler): Scheduler: killing slave 23488
kio (Slave): killing slave pid=23488 (http://localhost)
kio (KIOJob): Job::kill this=0x826c398 m_progressId=0 quietly=true
kio (KIOJob): CopyJob::slotResult() state=5
kio (KIOJob): stat file:/tmp/br79813.bin
kio (KIOJob): KIO::stat for resolving conflict on file:/tmp/br79813.bin
kio (KIOJob): addSubjob(0x8260b80) this = 0x889a258
kio (KIOJob): StatJob::slotStatEntry
kio (KIOJob): CopyJob::slotResult() state=6
kio (KIOJob): Observer::open_RenameDlg job=0x889a258
kio (KIOJob):                         progressId=101
Comment 10 Waldo Bastian 2004-08-01 17:20:17 UTC
The main problems is that the put job only detects that the file already exists when it tries to write the data (see the "error 12" part, which stands for ERR_FILE_ALREADY_EXIST) At that time it has already issued a "dataReq" signal.

The obvious fix would be to emit the error before it emits dataReq because then we actually have a chance to keep the slave that handles the "get" part of the request around till we have resolved the overwrite / rename issue.

That actually seems to fix the issue. Patch attached for the file-slave, other protocols would need to be checked as well.
Comment 11 Waldo Bastian 2004-08-01 17:21:36 UTC
Created attachment 6957 [details]
Patch for file io-slave
Comment 12 Kevin Puetz 2004-08-01 21:46:11 UTC
so am I the only one seeing this even when the destination does *not* already exist? (I am, even in 3.2.92). 

If I hit the submit button on that form, hit save in the resulting file, and put it on the desktop (where no file of the same name exists), I see it (as viewed from the server side) do another GET and thus write the wrong data out.

How do I generate a log like waldo posted to see why it's happening in that case?
Comment 13 Waldo Bastian 2004-08-02 14:26:10 UTC
Re #12: Configure kdelibs with debug output, edit kdelibs/kio/kio/job.cpp and remove the "//" from all lines that start with "//kdDebug" (Sometimes you have to remove the "//" from the next line as well)

Then make sure to do "make install" from kdelibs/kio (doing it in kdelibs/kio/kio doesn't work!)
Comment 14 Waldo Bastian 2004-08-02 14:28:00 UTC
CVS commit by waba: 

Return ERR_FILE_ALREADY_EXIST before sending dataReq (BR79813)
CCMAIL: 79813-done@bugs.kde.org


  M +10 -10    file.cc   1.151


--- kdelibs/kioslave/file/file.cc  #1.150:1.151
@@ -319,4 +319,13 @@ void FileProtocol::put( const KURL& url,
     }
 
+    if ( bOrigExists && !_overwrite && !_resume)
+    {
+        if (S_ISDIR(buff_orig.st_mode))
+            error( KIO::ERR_DIR_ALREADY_EXIST, dest_orig );
+        else
+            error( KIO::ERR_FILE_ALREADY_EXIST, dest_orig );
+        return;
+    }
+
     int result;
     QString dest;
@@ -336,13 +345,4 @@ void FileProtocol::put( const KURL& url,
             if (dest.isEmpty())
             {
-                if ( bOrigExists && !_overwrite && !_resume)
-                {
-                    if (S_ISDIR(buff_orig.st_mode))
-                      error( KIO::ERR_DIR_ALREADY_EXIST, dest_orig );
-                    else
-                      error( KIO::ERR_FILE_ALREADY_EXIST, dest_orig );
-                    return;
-                }
-
                 if (bMarkPartial)
                 {


Comment 15 Alain Knaff 2005-06-15 23:19:57 UTC
The bug has re-appeared in Konqueror 3.4.1, but this time with a vengeance: now *any* dialog displayed while servicing the POST will trigger the faulty behavior, not just the overwrite dialog. Yes, even the Open/Save dialog and then the dialog to chose a target file name are enough to trigger it.
Comment 16 Alain Knaff 2006-03-01 09:31:50 UTC
Checked again with Konqueror 3.5.1-4.2.fc3.kde on Fedora Core 3, and the bug is fixed now.

Just lets hope it won't re-appear again ;-)
Comment 17 Alain Knaff 2006-03-30 14:50:00 UTC
Checked in on Konqueror 3.5.1 on _SuSE_ and it was broken.

Also still broken on 3.5.2 on SuSE.

Comment 18 FiNeX 2008-04-04 00:58:29 UTC
On KDE4 (trunk) saving the file will start KGET which save an empty file.
Comment 19 Dawit Alemayehu 2011-12-05 19:00:31 UTC
The fixes committed for KIO slave reuse should also fix this problem so that there is no new GET request is performed when overwriting of an existing file is needed:

https://svn.reviewboard.kde.org/r/6271/