file overwriting when using CudaLister

[xaviermdq]

When I use CudaLister with a file and perform a search, the contents of the file are replaced by some sort of INI file.
Steps to reproduce the problem:

• Create a file named M.php with random PHP code but with the text M.php inside (a comment for example). I use:

<?php
// M.php
echo "Hello, World";
?>

• Make a backup copy of that file to other folder
• In TC, set in one panel the original M.php folder and in the other panel set the backup folder
• Use CudaLister on original file (F3 with file selected in TC)
• Press CTRL+F and search for M.php
• Press F3 and search again for M.php
• Press ESC
• Now, M.php file content is replaced with:

[SearchText]
0=M.php

More strange is that, sometimes, the file replaced is the backup file, not the file where I'm using CudaLister.
Sometimes, I have to make this steps 2 times to see the replacement.

[Alexey-T]

seems the patch from @springsunx is the reason. @springsunx , please see it.

[springsunx]

Is the CudaLister you are using 32-bit or 64-bit? 32-bit is prone to issues.

I cannot reproduce this problem.

Please provide some screenshots.

The program does output

[SearchText]
0=M.php

But it should be written to WINCMD.INI。

[xaviermdq]

Is the CudaLister you are using 32-bit or 64-bit? 32-bit is prone to issues.
CudaLister 1.8.1.0 + TC 10.52, 64 bits on Win 11 22H2 (10.0.22621.1702)

Please provide some screenshots.
I don't know what to screenshot. I repeated the instructions I gave before and the first time it didn't do the overwrite but the second time it did. Also, the file is overwritten regardless the situation of the other panel. So, the exact sequence I follow is:

1. Press F3 on file to view
2. Press CTRL+F to open Find dialog
3. Type some text that exist in file and press Enter
4. Press F3 to reopen Find dialog and press Enter to repeat the search with the same text
5. Press ESC to exit lister
6. Repeat 1 to 5
7. The file is overwritten
   I have noticed that if I press ESC and switch panels, what is overwritten is the selected file from the other panel. That is, it is as if it is taking the currently selected file to save the INI parameters.

[springsunx]

Is the CudaLister you are using 32-bit or 64-bit? 32-bit is prone to issues.
CudaLister 1.8.1.0 + TC 10.52, 64 bits on Win 11 22H2 (10.0.22621.1702)

Please provide some screenshots.
I don't know what to screenshot. I repeated the instructions I gave before and the first time it didn't do the overwrite but the second time it did. Also, the file is overwritten regardless the situation of the other panel. So, the exact sequence I follow is:

1. Press F3 on file to view
2. Press CTRL+F to open Find dialog
3. Type some text that exist in file and press Enter
4. Press F3 to reopen Find dialog and press Enter to repeat the search with the same text
5. Press ESC to exit lister
6. Repeat 1 to 5
7. The file is overwritten
   I have noticed that if I press ESC and switch panels, what is overwritten is the selected file from the other panel. That is, it is as if it is taking the currently selected file to save the INI parameters.

I still cannot reproduce this problem.

[xaviermdq]

I'll try a clean TC installation with only CudaLister

[elgonzo]

Having installed TC and CudaLister recently on my new Win11 box, i just also was affected by this problem.

I don't get how this issue could not be reproduced nor understood by the authors/maintainers, given @Alexey-T already directly pointed at the cause of it. Given the severity of the issue -- uncommanded overwriting of files (which is not a mere pedestrian usability issue, mind you), i am surprised and disappointed that nobody seems to have bothered to look at the code @Alexey-T already directly pointed at and study its behavior given the information in the issue report. The lack of action on such a severe issue is quite concerning to me as a user of this plug-in for several years now.

As @Alexey-T already indicated the issue is originating from @springsunx's patch, more specifically the SetLastFindText implementation.

First, lets see how SetLastFindText is being called, which gives insight into the conditions necessary to invoke SetLastFindText and thus reproduce the problem. SetLastFindText is being called in TfmMain.edKeyDown:

CudaLister/form_main.pas

Lines 436 to 443 in 0e1425a

	if (Key=VK_F3) and (Shift=[]) then
	begin
	//DoFind(true, true, Finder.OptCase, Finder.OptWords, '');
	Finder.StrFind:= GetLastFindText;
	if ed.TextSelected<>'' then begin
	Finder.StrFind:= ed.TextSelected;
	SetLastFindText(Finder.StrFind);
	end;

The F3 key is pressed (for invoking the Find dialog), and the current text selection in the CudaLister is checked. If the current text selection is not empty, SetLastFindText will be called with an argument value string that is the selected text.

And that's what SetLastFindText is doing:

CudaLister/form_main.pas

Lines 379 to 404 in 0e1425a

	function SetLastFindText(const AString: string) : string;
	var
	S: string;
	begin
	with TIniFile.Create(GetEnvironmentVariable('COMMANDER_INI')) do
	try
	S:= ReadString('SearchText', '0', '');
	if S='' then
	S:= ReadString('SearchText', 'RedirectSection', '')
	else
	WriteString('SearchText', '0', AString);
	if S='' then
	S:= ReadString('Configuration', 'AlternateUserIni', '');
	finally
	Free
	end;
	if FileExists(ExpandEnvironmentVariables(S)) then
	begin
	with TIniFile.Create(ExpandEnvironmentVariables(S)) do
	try
	WriteString('SearchText', '0', AString);
	finally
	Free
	end;
	end;
	end;

Note how in the latter half of the function it attempts writing to a an existing file whose name/path is denoted by the string S. What is the value of S?

If the TC INI file has already search terms in its [SearchText] sections, then the value of S will be the first search term. And if that first search term coincides with the name/path of an existing file (after env. var expansion), then --Boom!--, CudaLister is going to either modify that file if it so happen to be in an INI-compatible format, or otherwise it unceremoniously overwrites it. It's right there in plain sight in the SetLastFindText implementation @Alexey-T already referred to as prime suspect for this issue.

All you need to do to successfully reproduce the error is to make sure that the zero-indexed (i.e., the first) entry in the [SearchText] section is the name/path of an existing file, and boom!, that file will be overwritten by CudaLister when following the reproduction steps given in this issue thread.

So, to make the reproduction steps really clear, minimal, and perfectly reproducible:

1. Create a text file a.txt with some text content in some directory.
2. In the same directory, create another file foo.txt, also with some text content. Make sure its content doesn't look like an INI file (so as to observe what CudaLister is doing more easily).
3. Close TC.
4. Edit the [SearchText] section in TC's active INI file, so that the first search term (having the index zero) is the name of the file foo.txt, i.e.:

...
[SearchText]
0=foo.txt
...

5. Start TC again.
6. Navigate to the directory with the a.txt and foo.txt, if TC is not yet in this directory.
7. Open a.txt in the CudaLister.
8. Select some text.
9. Open the Find dialog with F3.
10. Boom! foo.txt just got hosed.

It's repeatable and reproducible every time (i mean, how could it not, looking at the SetLastFindText implementation)

(Now, in case the first search term in TC's [SearchText] INI section is not coinciding with the name/path of an existing file in the current directory, then the steps as outlined by @xaviermdq have to be taken. The first F3 invocation of the search dialog and searching for "M.php" will set the first search term in the [SearchText] section to 0=M.php, and a subsequent opening of the search dialog will then read "M.php" as the first search item and use it as file name to write on. Why this isn't reproducible reliably is perhaps -- and that's just my speculation -- a race condition where CudaLister has still a file handle open on the viewed M.php file elsewhere when SetLastFindText tries to write to it, essentially coincidentally preventing the uncommanded write to it.)

Hope this helps. The handling of this issue has given me pause however. Personally and sadly, i am not going to use CudaLister anymore, or perhaps i will stick with an older version; i don't know yet. This report of a severe issue should have gotten more attention, more than just @Alexey-T pointing out the culprit and then apparently nobody looking at and nobody actually verifying the behavior of the code base. Given the severity of the issue, the apparent inaction following the report is not something i am comfortable with, as little of relevance this possibly is to anyone but me.

[elgonzo]

Addendum/side note: While the function GetLastFindText does not cause files to be destroyed, it has a similarly broken implementation like SetLastFindText. It tries to read from an INI file whose name is given by the variable S. But the value of S would often be the first memorized search term (given GetLastFindText's current implementation), so why would it treat the search term text as the name/path of an INI file to read the search term from yet again? I can't see any reason to do that...

[pantantrollo]

Hope this helps. The handling of this issue has given me pause however. Personally and sadly, i am not going to use CudaLister anymore, or perhaps i will stick with an older version; i don't know yet. This report of a severe issue should have gotten more attention, more than just @Alexey-T pointing out the culprit and then apparently nobody looking at and nobody actually verifying the behavior of the code base. Given the severity of the issue, the apparent inaction following the report is not something i am comfortable with, as little of relevance this possibly is to anyone but me.

Hello,
Which version would be free of said error?

Thx

[elgonzo]

@pantantrollo

Hello,
Which version would be free of said error?

I can't say with certainty. I am a user myself, and so far have not yet tested older CudaLister versions.
But you can do this easily yourself. Download the previous version of CudaLister from https://github.com/Alexey-T/CudaLister/releases, install it, and do the reproduction steps with the a.txt and foo.txt files i mentioned in my first comment. If the issue is still reproducible with the previous version, download and install the next older version prior to that and test again.

[Alexey-T]

I will try to reproduce this soon (this month maybe). sorry for delay.

[Alexey-T]

let's test this fixed beta:
wlx_cudalister_1.8.2.0.zip

[pantantrollo]

let's test this fixed beta: wlx_cudalister_1.8.2.0.zip

Thx @Alexey-T , I'll try it

[elgonzo]

@Alexey-T,

thanks for the lightning-quick follow-up.

I tested the 64-bit version of wlx_cudalister_1.8.2.0.zip. The problem is fixed, as far as i can tell. (I haven't tested the 32-bit version nor with the INI-file redirects, though.)

Would you perhaps also fix the implementation of GetLastFindText before releasing the new CudaLister version to the public? It's not really a problem if you don't, as the current implementation of GetLastFindText can in the worst case only fill the search text box with an unexpected text. Which more or less is really just a rather cosmetic UX issue.

Just in case it is of interest, i have taken your modified SetLastFindText code from your last commit as a template for an improved GetLastFindText implementation. But note that i am not a Pascal programmer and don't have a Pascal development environment here at my side. I basically wrote it just visually. So it might (and quite possibly does) contain sloppy syntax errors due to my unfamiliarity with the Pascal language. Hence why i wouldn't dare putting it in a pull request and instead post it here as a code snippet for you to consider. Or feel free to not consider it without any worries; as i said in the last paragraph, the issue exhibited by the current GetLastFindText implementation is just of a cosmetic nature.

function GetLastFindText : string;
var
  TCIni, S: string;
begin
  TCIni := GetEnvironmentVariable('COMMANDER_INI');
  if TCIni='' then
  begin
    Result := '';
    Exit;
  end;

  with TIniFile.Create(TCIni) do
  try
    S:= ReadString('SearchText', '0', '');
    if S<>'' then
    begin
      Result := S;
      Exit;
    end;
    S:= ReadString('SearchText', 'RedirectSection', '');
    if S='' then
      S:= ReadString('Configuration', 'AlternateUserIni', '');
  finally
    Free
  end;
  if FileExists(ExpandEnvironmentVariables(S)) then
  begin
    with TIniFile.Create(ExpandEnvironmentVariables(S)) do
    try
      S:= ReadString('SearchText', '0', '');
    finally
      Free
    end;
  end;
  Result := S;
end;

[Alexey-T]

thanks for your help. why i did not answer for 3 months? see the date of your 1st post. it is very busy day in Ru. it was some coup in the military group in Ru.

[elgonzo]

I am not the OP :-)
Stay safe, mate!

[pantantrollo]

thanks for your help. why i did not answer for 3 months? see the date of your 1st post. it is very busy day in Ru. it was some coup in the military group in Ru.

For my part everything is ok, I don't usually ask for many explanations to those who selflessly contribute to the community :-)

Thanks