Fix using macro with regex in path parameter in fbtrace.conf by TreeHunter9 · Pull Request #8238 · FirebirdSQL/firebird

TreeHunter9 · 2024-09-03T07:38:26Z

Currently we cannot use macro with regex in path parameter because PathUtils::fixupSeparators(value.begin()); happens too early, and it will fix \1 to /1 before we replace it with regex group, so we get unexpected path.
On Windows, we just get an error, because PathUtils::fixupSeparators(value.begin()); will change all / to \, and it will cause expandPattern() to throw an exception.

TreeHunter9 · 2024-09-03T07:44:54Z

+		if (flags & FORCE_SLASHES_INSTEAD_OF_BACKSLASHES)
+		{
+			size_t pos = 0;
+			while ((pos = macro.find('\\', pos)) != String::npos)
+				macro[pos] = '/';
+		}
+		else
+		{
+			// Avoid incorrect slashes in pathnames
+			PathUtils::fixupSeparators(value.begin());
+			PathUtils::fixupSeparators(macro.begin());
+		}


I'm trying to keep the old implementation, so I added FORCE_SLASHES_INSTEAD_OF_BACKSLASHES flag, but maybe we can just remove these PathUtils::fixupSeparators() because it will happen in PATH_PARAMETER after all parsing steps anyway?

Please provide a sample of causing problems trace configuration

database = %[\\/](employee).fdb { enabled = true log_filename = $(root)\1.log log_connections = true time_threshold = 0 }

Log filename should be fb_root_path/employee.log, but we get fb_root_path/1.log.

On Windows it doesn't even get created because it's trying to parse backslashes as part of regex pattern (\1, \2, etc) but encounters \d[ev] in the file path and throws an error:

DESKTOP-86JTPHF Tue Sep 3 10:37:29 2024 Trace plugin fbtrace returned error on call trace_create. error while parsing trace configuration line 195, element "log_filename": pattern is invalid E:\dev\MyFirebird\firebird\output_x64_debug\1.log

AlexPeshkoff

I'm afraid suggested solution is not complete. Imagine someone types (in windows):
log_filename = $(root)\audit-logs\1.log
Some problems are hardly avoidable when same char is used for 2 different purposes, but lets try to minimize them.

I suggest to use something like REGEXP_SUPPORT instead FORCE_SLASHES_INSTEAD_OF_BACKSLASHES and carefully parse both value and macro (moreover, value should be parsed when flag is set even when no macro-subst is done) in order to replace backslash with slash when next char is not a digit. At the first glance it appears more generic. Certainly if someone called directory '1' and tries to use it for logs I do not know how to guess what to do with that.

aafemt · 2024-09-05T14:52:06Z

Some problems are hardly avoidable when same char is used for 2 different purposes, but lets try to minimize them.

If you are ready to review the initial design, I would suggest to use $(1) for subpattern substitutions.

AlexPeshkoff · 2024-09-05T15:33:21Z

On 9/5/24 17:52, Dimitry Sibiryakov wrote: Some problems are hardly avoidable when same char is used for 2 different purposes, but lets try to minimize them. If you are ready to review the initial design, I would suggest to use |$(1)| for subpattern substitutions.

Looks as a minimum interesting - but discussion is devel is needed for such change.

TreeHunter9 · 2024-09-05T16:32:31Z

I'm afraid suggested solution is not complete. Imagine someone types (in windows):
log_filename = $(root)\audit-logs\1.log
Some problems are hardly avoidable when same char is used for 2 different purposes, but lets try to minimize them.

Oh, for some reason I thought only / was allowed as a separator in the path. Now I looked more into it and found out that we should use \\ for backslashes in path (as it says in fbtrace.conf), and here is code snippet from TraceCfgReader::expandPattern:

firebird/src/utilities/ntrace/TraceConfiguration.cpp

Lines 291 to 297 in 72cc088

    
           if (c == '\\') 
        
           { 
        
           	// Kill one of the backslash signs and loop again 
        
           	valueToExpand.erase(pos, 1); 
        
           	pos++; 
        
           	continue; 
        
           }

So I guess the problem only remains on Linux because of PathUtils::fixupSeparators(value.begin()); will fixup \1 regex pattern.
I will do another test tomorrow.

…upport

TreeHunter9 · 2024-09-06T09:13:14Z

I'm afraid suggested solution is not complete. Imagine someone types (in windows):
log_filename = $(root)\audit-logs\1.log

We should write path like this: $(root)audit-logs\\\1.log, $(root) already have \\ at the end, and audit-logs\\\1.log so we can translate it into audit-logs\employee.log. So I guess we don't need to change the semantics of the regexp.
So we really only have one problem, where macro insertion fixes all regexp patterns (on Windows macro insertion uses incorrect backslashes for path) and this patch should fix that.

AlexPeshkoff · 2024-09-06T11:26:37Z

Macro in config design supposed to provide smart behavior when merging paths, i.e. one need not know that $(root) already has \ at the end. In linux having double dir separators one after another (something like /usr//bin instead /usr/bin) is not dangerous but I'm not sure for other OS-es. Anyway, resulting file name may get visible to user somewhere else, i.e. I prefer to have correctly merged paths, not relying on presence of // at the end. Also we always tried to have config format as OS independent as possible, i.e. one should be able to have
$(root)\audit-logs\\1.log on linux or
$(root)/audit-logs/\1.log on windows (may be as a result of file copy when changing OS).

TreeHunter9 · 2024-09-09T13:56:25Z

Tested on these file paths:

Windows and Linux:
$(root)\1.log -> /path/to/root/employee.log
$(root)/\1.log -> /path/to/root/employee.log
$(root)\\\1.log -> /path/to/root/employee.log
$(root)\\1.log -> /path/to/root/1.log

Only Linux:
/$(root)/\1.log -> /path/to/root/employee.log
\\$(root)/\1.log -> /path/to/root/employee.log

AlexPeshkoff · 2024-09-09T15:07:13Z

I suggest you backport changes to previous versions

dyemanov · 2024-09-09T16:17:15Z

I will backport it.

Fix using macro with regex in path parameter in fbtrace.conf

72cc088

TreeHunter9 commented Sep 3, 2024

View reviewed changes

AlexPeshkoff requested changes Sep 5, 2024

View reviewed changes

Replace single backslash with a double backslash in path for regexp s…

8e7d337

…upport

Fix smart path merging in fbtrace.conf

db493ea

TreeHunter9 requested a review from AlexPeshkoff September 9, 2024 13:58

AlexPeshkoff self-assigned this Sep 9, 2024

AlexPeshkoff approved these changes Sep 9, 2024

View reviewed changes

AlexPeshkoff merged commit 9f5b5bf into FirebirdSQL:master Sep 9, 2024

dyemanov assigned TreeHunter9 Sep 9, 2024

dyemanov added type: bug affect-version: 6.0 Initial fix-version: 6.0 Alpha 1 affect-version: 5.0.0 fix-version: 5.0.2 affect-version: 5.0.1 affect-version: 4.0.5 fix-version: 4.0.6 labels Sep 9, 2024

TreeHunter9 deleted the master-macro-and-regex-in-trace-conf branch September 10, 2024 06:37

mrotteveel added the rlsnotes60: no Intentionally not added to the Firebird 6.0 release notes. label Mar 2, 2026

Uh oh!

Conversation

TreeHunter9 commented Sep 3, 2024

Uh oh!

TreeHunter9 Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexPeshkoff Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TreeHunter9 Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

TreeHunter9 Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

AlexPeshkoff left a comment

Choose a reason for hiding this comment

Uh oh!

aafemt commented Sep 5, 2024

Uh oh!

AlexPeshkoff commented Sep 5, 2024 via email

Uh oh!

TreeHunter9 commented Sep 5, 2024

Uh oh!

TreeHunter9 commented Sep 6, 2024

Uh oh!

AlexPeshkoff commented Sep 6, 2024

Uh oh!

TreeHunter9 commented Sep 9, 2024

Uh oh!

AlexPeshkoff commented Sep 9, 2024

Uh oh!

dyemanov commented Sep 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TreeHunter9 Sep 3, 2024 •

edited

Loading

AlexPeshkoff Sep 3, 2024 •

edited

Loading