mattslay / GoFish

GoFish is an advanced code search tool for fast searching and replacing of Visual FoxPro source code.
17 stars 16 forks source link

The Delete feature on the Search History form can delete all files and folders on the entire disk (in certain scenarios...) #9

Closed grkncl closed 3 years ago

grkncl commented 3 years ago

Here is the initial comment @grkncl left to report this issue, then he seems to have deleted the text later, so I have added it back to have it on record:

You fool! All folders and files inside entire disk have been deleted because of your stupid app. Check the commands you made to clear "history" with the "delete" command.

@mattslay

mattslay commented 3 years ago

Well @grkncl , please help me understand exactly what you are referring to. Are you referring to the "Delete" button on the bottom of the Search History form?

If so, here is the code, so let's see what could have gone wrong...

If the problem is indeed with the GoFish code, then we need to figure it out... I see a reference to gf_SearchHistory.SearchHistoryFolder which should be a folder path that gets passed to the RemoveFolder() function.

Can you tell me exactly what your search history folder and history set was??

Delete Button Click() method:

lcPath = Alltrim(gf_SearchHistory.SearchHistoryFolder)

If RemoveFolder(m.lcPath)
    Delete In gf_SearchHistory
Else
    Messagebox('Unable to delete folder "' + m.lcPath + '"', 16, 'Unable to delete folder')
Endif

Here is the code for the RemoveFolder() function that is called above. Maybe we need to add some testing logic to the passed path name so this error cannot happen. Perhaps if an empty string is passed that could lead to a problem, and that scenario is not tested.

Procedure RemoveFolder(lcFolderName)

Procedure RemoveFolder(lcFolderName)
    Local laFiles[1], lcFileName, lcFileNameWithPath, llFailure, lnFileCount, lnI, loException

    Declare Integer SetFileAttributes In kernel32 String, Integer
    lcFolderName = Trim(m.lcFolderName)

    Try
        lnFileCount = Adir(laFiles, m.lcFolderName + '\*', 'DH')
        For lnI = 1 To m.lnFileCount
            lcFileName = m.laFiles[m.lnI, 1]
            If Left(m.lcFileName, 1) # '.'
                lcFileNameWithPath = m.lcFolderName + '\' + m.lcFileName
                SetFileAttributes(m.lcFileNameWithPath, 0)
                If 'D' $ m.laFiles[m.lnI, 5] && directory?
                    RemoveFolder(m.lcFileNameWithPath)
                Else
                    Delete File(m.lcFileNameWithPath)
                Endif
            Endif
        Endfor
        Rmdir(m.lcFolderName)

    Catch To m.loException
        llFailure = .T.
    Endtry

    Return m.llFailure = .F.

EndProc

image

bturski commented 3 years ago

@mattslay - thank you for volunteering your time to investigate this issue and maintain this extremely useful, free, open-source tool.

mattslay commented 3 years ago

@grkncl Sir, you have indeed found an unfortunate bug. I will fix it, and I'm glad it has been discovered, sadly at your expense and loss. I truly regret that.

Notwithstanding, you need to cease your insulting of me. This is a very well-written and useful app that has served many VFP developers for over a decade. You have found one issue, albeit admittedly a painful one.

Your anger has brought you to foolishness, which is a worse position than you were in when you had only lost files on your disk.

DougHennig commented 3 years ago

@grkncl Matt gave his time freely to create an extremely useful FREE utility that I use all the time. It's unfortunate that it had a bug that caused you problems and I am sure Matt regrets that. However, attacking him personally is unprofessional and has no place in a forum like this.

mattslay commented 3 years ago

@grkncl Can I ask you some questions to help me understand something?

  1. Did you indeed have some records in the Search History grid when the form launched?

  2. If so, how did the form get into a state where no record was selected in the grid? I have tried to accomplish this on my machine, and I cannot get the grid to have no row selected.

Of course I understand what guard code needs to be added to protect against a mass-delete situation now that you have reported it, but I am still curious about # 1 and # 2 above in your specific case.

grkncl commented 3 years ago

@mattslay I tried the application for the first time. I made a few test calls. then I started deleting them. I guess I hit the "delete" button again without realizing it was the last record. the application was locked for a few minutes. After I terminated the entire hard disk was wiped.

mattslay commented 3 years ago

@grkncl Thanks for this info. I can see now how this could lead to the UI grid being empty. I will test against this scenario as well, and disable the the Delete button if there are no more records in the grid. And of course, I will add the important guard code in the RemoveFolder() so that it will simply Return out if the passed parameter is empty.

I will be posting an update on Tues 2021-02-23 or Wednesday 2021-03-24 through the Thor distribution channel, and I will be pushing the updated code to GitHub with full changelog notes.

@grkncl - I am truly sorry this unfortunate thing happened to you. Indeed when writing that code, I never imagined that the UI would allow the user to pass an empty string to the function, nor what the horrible results would be if they did. I cannot apologize enough for this bug.

Regardless of this unfortunate oversight on my part, I encourage you to further explore the app as many people have found it to be a very helpful tool for searching their code base for the past 11 years.

mattslay commented 3 years ago

@grkncl You said to @DougHennig "I hope it happens to you."

@grkncl Sir, I now invite you to simply leave the entire FoxPro community forever. This kind malicious spewing is absolutely not welcome here. You are obviously not one of us.

Goodbye!

mattslay commented 3 years ago

This issue has been fixed and source code pushed to GitHub as ver 5.0.164. (bug was fixed within 13 hours of being reported).

I am working with the VFPx Team leaders to get the update released via Thor Check For Updates.

GerhardSc commented 3 years ago

Hello Matt! I'm a happy user of your tool for years now. @grkncl: No backup - no mercy.

grkncl commented 3 years ago

okay cringing @GerhardSc . I hope it happens to you as soon as possible. you restore terabyte backups

mattslay commented 3 years ago

I have blocked user @grkncl from all of my GitHub repos.

Twice he has wished "I hope it happens to you" on other VFP folks here in the comments of this issue. There is no place for that poison here.

Indeed there was an error in my coding of the RemoveFolder() method, that I did not realize what potential issue could be from a passed empty string, and I regret that I did not recognize this potential at the time. Regardless, he took his replies to an unacceptable level by wishing the same havoc on other more reasonable users.

GerhardSc commented 3 years ago

Well done, Matt! Never thought that it would be necessary here.

Gerhard Schmidbauer

mattslay commented 3 years ago

This message was emailed to the GoFish and Thor Google Groups on 2021-03-24:

Hi GoFish users! I need to alert everyone about a very critical bug in GoFish that was reported last Friday March 19 2021. The error is only in a certain condition when using the "Delete" button at the bottom of the Search History form. Sadly, if this specific error condition is triggered, GoFish will begin deleting all files and folders at the root level of the disk. Yes, all of them! Please read on...

The error condition is that if you use the Delete button to delete all of the Search History records from the grid, even after the last record is deleted, the Delete button will still remain enabled, and if you click it again, while there are no rows in the grid, it will pass an empty string to the GF utility function named RemoveFolder(tcFolder), and the code in that method does not test for empty string, and it uses the ADir() function on that empty string, which results in the starting folder for deletion being the root folder of the disk, and it iterates through all the folders from there attempting to delete them and the contents. This actually happened to a new user who was testing GF and then deleted his initial 3 or 4 Search History records, then he clicked the Delete button again, and the mass-delete unfolded from there. He was (is), understandably, very upset with me (just read the Issue #9 comment stream and you will see.)

The error condition was reported first hand by user @grkncl as reported in Issue #9 on the GitHub repo. If you want to read all the comments about his experience with this issue you can find it here: https://github.com/mattslay/GoFish/issues/9

The good news is that the error has been safely and thoroughly fixed in the latest release of GoFish (ver 5.0.169) which you can get through Thor Check For Updates. Other features have been added in this release also, so I recommend upgrading as soon as you can. Of course, the full and latest source code is available at: https://github.com/mattslay/GoFish

You can see a the recent updates listed in the Change Log section at the bottom of the page.