View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0002102Far ManagerCommand Line/Executepublic2012-05-16 22:582012-05-21 19:38
Reporterddv Assigned ToDrKnS  
PrioritynormalSeveritytweakReproducibilityalways
Status closedResolutionfixed 
Platformx64OSWindowsOS Version7
Product Version3.0 
Fixed in Version3.0 
Summary0002102: Path environment variable not processed correctly isf it contains empty strings between semicolons
DescriptionIf I have an empty component in the path environment variable, Far fails to parse that path completely. In particular, a wrong file may be executed as a result.
Steps To ReproduceHave your Path environment variable contain an empty component (in other words, two semicolons in a row, for example, "C:\dir1;;C:\dir2"). Have a build.cmd in C:\dir1 and a build.exe in C:\dir2, and try to execute "build" in Far's command line. According to the cmd.exe's logic, "C:\dir1\build.cmd" should be executed. Instead, Far executes "build.exe".
Additional InformationI have a fix for this that I would like to contribute but am not sure about the contribution process.
Essentially, I add a "SkipEmptyLines" bool field to UserDefinedList and have it skip any empty string item in the list when parsing it. Then I set that field every time the "Path" environment variable's value is being parsed.
Can somebody point me to the instructions on how to contribute fixes to Far Manager?
TagsNo tags attached.
Build2686

Activities

DrKnS

2012-05-16 23:54

administrator   bugnote:0009218

Please explore existing code more carefully, before adding new - UserDefinedList already has a flag to skip empty items (though it's name is not so obvious) so all you need is a replace strings like "UserDefinedList PathList" with "UserDefinedList PathList(0, 0, ULF_ACCOUNTEMPTYLINE);" in all places where %path% is processed.
Maybe it's even better to change a default behaviour and always silently skip empty items.
As for contribution process - when you done editing, the easiest way is to invoke "svn diff > patch.diff" in your local copy and attach patch.diff to appropriate bugrtacker issue. See HACKING-EN file for details.

ddv

2012-05-17 00:59

reporter   bugnote:0009219

Actually, I saw the "AccountEmptyItems" flag and even thought of using it originally, but it was not clear to me that its intention is to silently skip empty items: it still increments the item's index which I thought has some non-obvious consequences. Now that you mentioned it, it appears the index is not used for anything but sorting by it so skipping an index value does not affect any behavior.
Another thing not entirely clear to me about this flag is this additional condition: (AccountEmptyLine && CurList==List && !item.index).
Looks like this will not allow an empty item to be the first one in the list. Is this intended? For the behavior to match cmd.exe, this condition should be removed. Is it OK to remove it for all UserDefinedList instances that use AccountEmptyLine?
You also suggest making this "skip empty lines" default. Then do you know which of the UserDefinedList instances should specifically not use it?

DrKnS

2012-05-17 18:29

administrator   bugnote:0009224

> Is this intended?

no, it's just an old & buggy code.

see build 2683.

ddv

2012-05-18 01:27

reporter  

patch.diff (5,701 bytes)    
Index: udlist.hpp
===================================================================
--- udlist.hpp	(revision 8066)
+++ udlist.hpp	(working copy)
@@ -129,6 +129,7 @@
 		void SetDefaultSeparators();
 		const wchar_t *Skip(const wchar_t *Str, int &Length, int &RealLength, bool &Error);
 		static int CmpItems(const UserDefinedListItem **el1, const UserDefinedListItem **el2);
+		static void UpdateRemainingItem(UserDefinedListItem *elRemaining, const UserDefinedListItem *elDeleted);
 
 		TArray<UserDefinedListItem> Array;
 		size_t CurrentItem;
Index: array.hpp
===================================================================
--- array.hpp	(revision 8066)
+++ array.hpp	(working copy)
@@ -43,12 +43,6 @@
 THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */
 
-#ifdef __GNUC__
-typedef int __cdecl(*TARRAYCMPFUNC)(const void *el1,const void *el2);
-#else
-typedef int (*TARRAYCMPFUNC)(const void *el1,const void *el2);
-#endif
-
 template <class Object>
 class TArray
 {
@@ -78,7 +72,7 @@
 		size_t getIndex(const Object &item, int start=-1);
 
 		// ���������� �������. Offset - ������ ���� ������� ���������
-		void Sort(TARRAYCMPFUNC user_cmp_func=nullptr,size_t Offset=0);
+		void Sort(int (__cdecl *user_cmp_func)(const Object **el1,const Object **el2)=nullptr,size_t Offset=0);
 
 		// �������� ������ - ������ �������� �������� ���������,
 		/*
@@ -86,8 +80,10 @@
 			���� ��������� ������� �� ������������.
 			���� Pack() ����� Sort(nullptr) �������� � ���������
 			����������
+
+			����� user_updateRemainingItem_func ���������� ��������� �������� � elRemaining ��� �������� ����������� �� ��� elDeleted
 		*/
-		bool Pack();
+		bool Pack(void (__cdecl *user_updateRemainingItem_func)(Object *elRemaining, const Object *elDeleted)=nullptr);
 
 	public: // inline
 		size_t getSize() const { return Count; }
@@ -146,19 +142,19 @@
 }
 
 template <class Object>
-void TArray<Object>::Sort(TARRAYCMPFUNC user_cmp_func,size_t Offset)
+void TArray<Object>::Sort(int (__cdecl *user_cmp_func)(const Object **el1,const Object **el2),size_t Offset)
 {
 	if (Count)
 	{
 		if (!user_cmp_func)
-			user_cmp_func=reinterpret_cast<TARRAYCMPFUNC>(CmpItems);
+			user_cmp_func=CmpItems;
 
-		far_qsort(reinterpret_cast<char*>(items+Offset),Count-Offset,sizeof(Object*),user_cmp_func);
+		far_qsort(items+Offset,Count-Offset,sizeof(Object*),(int (__cdecl *)(const void *, const void *))user_cmp_func);
 	}
 }
 
 template <class Object>
-bool TArray<Object>::Pack()
+bool TArray<Object>::Pack(void (__cdecl *user_updateRemainingItem_func)(Object *elRemaining, const Object *elDeleted))
 {
 	bool was_changed=false;
 
@@ -166,6 +162,8 @@
 	{
 		if (*items[index-1]==*items[index])
 		{
+			if (user_updateRemainingItem_func)
+				user_updateRemainingItem_func(items[index-1], items[index]);
 			deleteItem(index);
 			was_changed=true;
 			--Count;
@@ -250,7 +248,7 @@
 template <class Object>
 int __cdecl TArray<Object>::CmpItems(const Object **el1, const Object **el2)
 {
-	if (el1==el2)
+	if (*el1==*el2)
 		return 0;
 	else if (**el1==**el2)
 		return 0;
Index: help.cpp
===================================================================
--- help.cpp	(revision 8066)
+++ help.cpp	(working copy)
@@ -1865,7 +1865,7 @@
 	}
 
 	// ��������� �� ��������
-	HelpList.Sort(reinterpret_cast<TARRAYCMPFUNC>(CmpItems),OldStrCount);
+	HelpList.Sort(CmpItems,OldStrCount);
 
 	// $ 26.06.2000 IS - ���������� ���� � ������ �� f1, shift+f2, end (������ ��������� IG)
 	AddLine(L"");
Index: changelog
===================================================================
--- changelog	(revision 8066)
+++ changelog	(working copy)
@@ -1,3 +1,7 @@
+drkns 17.05.2012 17:10:00 -0700 - build 2684
+
+1. ����������� �������� ULF_UNIQUE ��� ULF_SORT � �������� ����������� � �������������� ����� ������ � TArray.
+
 drkns 17.05.2012 20:42:33 +0200 - build 2684
 
 1. �������� 2683.
@@ -5,7 +9,7 @@
 drkns 17.05.2012 20:18:36 +0200 - build 2683
 
 1. ����������� UserDefinedList. ����� ������������ ����� �� ������ ������, ����� ����� ����������� ������ �� ������.
-   ��� �� 0002102: Path environment variable not processed correctly isf it contains empty strings between semicolons
+   ��� �� 0002102: Path environment variable not processed correctly if it contains empty strings between semicolons
 
 w17 17.05.2012 13:38:16 +0400 - build 2682
 
Index: udlist.cpp
===================================================================
--- udlist.cpp	(revision 8066)
+++ udlist.cpp	(working copy)
@@ -288,11 +288,11 @@
 		if (Flags.Check(ULF_UNIQUE))
 		{
 			Array.Sort();
-			Array.Pack();
+			Array.Pack(UpdateRemainingItem);
 		}
 
 		if (!Flags.Check(ULF_SORT))
-			Array.Sort(reinterpret_cast<TARRAYCMPFUNC>(CmpItems));
+			Array.Sort(CmpItems);
 		else if (!Flags.Check(ULF_UNIQUE)) // ���� �� ���������� ��� ���������������
 			Array.Sort();
 
@@ -312,7 +312,7 @@
 int CDECL UserDefinedList::CmpItems(const UserDefinedListItem **el1,
                                       const UserDefinedListItem **el2)
 {
-	if (el1==el2)
+	if (*el1==*el2)
 		return 0;
 	else if ((**el1).index==(**el2).index)
 		return 0;
@@ -322,6 +322,12 @@
 		return 1;
 }
 
+void CDECL UserDefinedList::UpdateRemainingItem(UserDefinedListItem *elRemaining, const UserDefinedListItem *elDeleted)
+{
+	if (elRemaining->index > elDeleted->index)
+		elRemaining->index = elDeleted->index;
+}
+
 const wchar_t *UserDefinedList::Skip(const wchar_t *Str, int &Length, int &RealLength, bool &Error)
 {
 	Length=RealLength=0;
patch.diff (5,701 bytes)   

ddv

2012-05-18 01:28

reporter   bugnote:0009225

Thanks for fixing this. I looked at your code and found a potentially incorrect behavior still. The current implementation of the ULF_UNIQUE (without ULF_SORT) flag produces a "non-deterministic" order, despite its apparent intention to sort the "packed" array by the original indexes. I have specifically tried setting my path to "2,1,2,1" and executing some command line. What I expected to see as a result of PathList.Set(strPathEnv) was an array containing strings "2" followed by "1"; instead, I got the reverse order. The problem is that qsort does not guarantee the order of the items that compare as equal. One "hack" I can imagine to address this is change the implementation of TArray<>::Pack to assign the index of the previous item to the index of the item being deleted if the latter one is smaller.
I made this change with no doubts caused by my non-familiarity with the code, so attaching this fix here.

DrKnS

2012-05-18 07:54

administrator   bugnote:0009227

ok, 2686.

ddv

2012-05-21 18:35

reporter   bugnote:0009250

Thanks.

Issue History

Date Modified Username Field Change
2012-05-16 22:58 ddv New Issue
2012-05-16 23:54 DrKnS Note Added: 0009218
2012-05-17 00:59 ddv Note Added: 0009219
2012-05-17 18:29 DrKnS Note Added: 0009224
2012-05-17 18:29 DrKnS Assigned To => DrKnS
2012-05-17 18:29 DrKnS Status new => feedback
2012-05-18 01:27 ddv File Added: patch.diff
2012-05-18 01:28 ddv Note Added: 0009225
2012-05-18 01:28 ddv Status feedback => assigned
2012-05-18 07:54 DrKnS Note Added: 0009227
2012-05-18 07:54 DrKnS Status assigned => feedback
2012-05-21 18:35 ddv Note Added: 0009250
2012-05-21 18:35 ddv Status feedback => assigned
2012-05-21 18:35 ddv Status assigned => resolved
2012-05-21 19:38 DrKnS Build => 2686
2012-05-21 19:38 DrKnS Status resolved => closed
2012-05-21 19:38 DrKnS Resolution open => fixed
2012-05-21 19:38 DrKnS Fixed in Version => 3.0