View Issue Details

IDProjectCategoryView StatusLast Update
0035991LazarusLCLpublic2020-09-12 21:30
ReporterMartin Friebe Assigned ToOndrej Pokorny  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Platform64bit IntelOSwin 10 
Product Version2.1 (SVN) 
Summary0035991: Crash (sigsegv) or Exception caused by TProcessUtf8
DescriptionCompiling either
 - LazUtils,
 - or the RTL
with -O4 will cause TProcessUtf8 to fail.

It can either
- raise an exception
- write to memory out of bounds, causing random errors (including SigSegv) at a later time.

This is caused by trying to "guess" the byte offset of a private field and modify it.

However with -O4 FPC may change the order of fields.

This is also extremely unstable with regards to future changes in fpc.
This may also fail, if WPO is used (not verified).


This is a result of the fix for 0028991 revision 50595
TagsNo tags attached.
Fixed in Revisionr63584, r63708
LazTarget-
WidgetsetWin32/Win64
Attached Files

Relationships

related to 0028991 resolvedMattias Gaertner Lazarus IDE cannot compile projects with FPC if path has national symbols 
related to 0032055 resolvedMichael Van Canneyt FPC TProcess doesn't respect the poNoConsole option 

Activities

Martin Friebe

2019-08-21 00:19

manager   ~0117754

Attempt to minimize crashes in
Revision: 61736
Date: 21 August 2019 00:16:37
Try to minimize (NOT a fix) the risks of TProcessUtf8 crashes. Crash introduced in r50595 for issue 28991 / See also issue 035991

This may still not catch everything. And especially there in knowing what will happen with future fpc releases

It will also still leave the process to fail with an exception, where it should actually work.

Marco van de Voort

2020-07-14 12:49

manager   ~0124003

A lot of code of utf8process has flowed into FPC 3.2.0 TProcess. The need to hack this into utf8process on this scale might not be necessary any more.

Bart Broersma

2020-07-14 13:51

developer   ~0124006

Last edited: 2020-07-14 13:55

View 2 revisions

Unfortunately we also still need to support fpc 3.0.4.
For 3.2+ couldn't we use ProcessUnicode unit?

Sven Barth

2020-07-14 16:15

manager   ~0124011

You could always enable/disable parts required for 3.0.4 using {$IF FPC_FULLVERSION < 30200} or so...

Juha Manninen

2020-07-14 23:38

developer   ~0124020

Last edited: 2020-07-14 23:40

View 2 revisions

FYI, in r63563 I deleted the very old version of TProcessUtf8 code that was needed for FPC 2.x.

@Martin, as Marco noted. Can we use TProcess code also for Windows?
What happens if you remove the UseTProcessW define in unit UTF8Process? Then Window uses the same code that Unix related systems use. It does not override anything, it only adds procedure ParseCmdLine() .

Martin Friebe

2020-07-15 00:54

manager   ~0124022

I actually do not know, why we moved away from the old "UseOldTProcess"

I did not add the entire TProcessClassTemplate hack into private field. I only added some checks to make it less likely that random other memory is overwritten, leading to crashes at random times.

If TProcess now handles utf8 => great.
I have not tested that. I also have no list of tests that would need to be performed.

Bart Broersma

2020-07-15 11:26

developer   ~0124034

Last edited: 2020-07-15 22:51

View 2 revisions

With unit ProcessUnicode I cann execute 草莓.exe (I'm on Dutch locale) from within Lazarus.
(The phrase '草莓.exe' is obtained from a TEdit, so it is UTF-8 encoded)

Marco van de Voort

2020-07-15 22:52

manager   ~0124068

FPC 3.2.0 is an superset of various TProcess and Runcommand variants that I found, of which tut8process was the largest one. An attempt was also made to make the main "Runcommand"/"wiki large output" event loop reusable by parametrizing with events. (see e.g. https://forum.lazarus.freepascal.org/index.php/topic,50525.msg368880.html#msg368880 for an example)

Processunicode is mostly for when defaultcodepage<>utf8. If defaultcodepage is utf8, process afaik also should work. Work was done on making the streams utf8 clean (but probably only if default codepage =utf8). "Tprocess.ReadInputStream" is now also a non blocking method to read a stream

There are some gotchas that came up with RC1 that should be fixed in final. See poRunIdle, podetached.

Let me know if there are problems, then we'll at least fix this in 3.2.1/2. Starting with making those handles protected I guess. Please next time somebody makes such hack, FILE A REPORT WITH FPC to make use aware of it.

Juha Manninen

2020-07-16 12:10

developer   ~0124083

Is it OK to commit the this patch?

@Bart, I guess you can execute 草莓.exe also with unit Process.
BTW, using unit ProcessUnicode gives an error here (not found) although it is in the same directory with unit Process.
0001-LazUtils-Don-t-override-TProcess.Execute-for-TProces.patch (936 bytes)   
From 56f468e3792bcd19a18b6594aba16f1548660a08 Mon Sep 17 00:00:00 2001
From: Juha <juha.manninen62@gmail.com>
Date: Thu, 16 Jul 2020 11:56:59 +0300
Subject: [PATCH] LazUtils: Don't override TProcess.Execute for TProcessUTF8
 starting from FPC 3.2.

---
 components/lazutils/utf8process.pp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/components/lazutils/utf8process.pp b/components/lazutils/utf8process.pp
index 83390f944..45020e1e3 100644
--- a/components/lazutils/utf8process.pp
+++ b/components/lazutils/utf8process.pp
@@ -14,14 +14,16 @@ unit UTF8Process;
 {$mode objfpc}{$H+}
 
 {$IFDEF MSWINDOWS}
+{$IF FPC_FULLVERSION < 30200}
   {$DEFINE UseTProcessW}
 {$ENDIF}
+{$ENDIF}
 
 interface
 
 uses
   Classes, SysUtils, Process,
-  {$IF defined(UseSeparateTProcessW) or defined(UseTProcessW)}
+  {$IFDEF UseTProcessW}
   pipes,
   {$ENDIF}
   FileUtil, LazFileUtils, LazUTF8, LazUtilsStrConsts;
-- 
2.27.0

Bart Broersma

2020-07-16 22:44

developer   ~0124103

> I guess you can execute 草莓.exe also with unit Process.
Much to my surpise, it seems you can indeed.
I distinctly remeber you could not use TProcess with unicode outside your current codepage.

> BTW, using unit ProcessUnicode gives an error here (not found) although it is in the same directory with unit Process.

This:
uses
  Classes, SysUtils, Forms, Controls, Graphics, Dialogs, StdCtrls,
  Process, ProcessUnicode;

compiles out of the box for me with fpc 3.2.0 (32-bit) on Win10-64.

Bart Broersma

2020-07-16 23:16

developer   ~0124105

Maybe we should deprecate ProcessUtf8 unit for fpc >= 3.2?

Juha Manninen

2020-07-17 10:13

developer   ~0124115

Last edited: 2020-07-17 15:39

View 4 revisions

> Much to my surpise, it seems you can indeed.

It is because of the improvements Marco mentioned, I guess.
Apparently ProcessUnicode is not built in Unix/Linux libraries. Codetools finds it by source code.
Anyway it is not needed as Marco explained.

> Maybe we should deprecate ProcessUtf8 unit for fpc >= 3.2?

Then method ParseCmdLine must be changed into a global procedure.
It does about the same thing as the deprecated CommandLine property of TProcess. Why is CommandLine deprecated? It is useful sometimes. However we should not change code to use deprecated stuff.

I applied my proposed change in r63584. Now with fpc >= 3.2 code from TProcess is used directly. Please test.

Sven Barth

2020-07-17 16:25

manager   ~0124124

The CommandLine property was deprecated, because it's easier and less error prone to compose a valid CommandLine from the Executable and Parameters properties for platforms like Windows than to split a CommandLine into Executable and Parameters for *nix platforms. There had been various bug reports about CommandLine and thus it was decided instead of fixing it to prefer Executable and Parameters instead.

Marco van de Voort

2020-07-18 16:47

manager   ~0124145

Last edited: 2020-07-18 16:47

View 2 revisions

As Sven says. The problem was also that people came with constantly more elaborate quoting schemes with the "works on the bash commandline" as comparison. That was simply not sustainable. Separating exe and parameters also makes clear it is not a shell emulation.

I think that deprecation is btw already in 2.6.2 or 2.6.4 or so.

Sven Barth

2020-07-20 07:38

manager   ~0124182

I just checked: it was added with 2.6.0. :)

Bart Broersma

2020-07-20 11:28

developer   ~0124186

So, we better ditch the use of CommandLine altogether.

Marco van de Voort

2020-07-20 12:28

manager   ~0124187

Last edited: 2020-07-20 12:35

View 2 revisions

Yes. I would keep TProcessUtf8 for a least a cycle though. A lot of code is new, and issues that need to be worked around might still pop up.

I also think a lot of the TProcess code should be removed from the "executing external programs" page. It is outdated and very basic. (large output, but blocking), and it is still copied far too often.

It should be replaced by more specific examples that using RunCommandLoop and readinputstream.

Juha Manninen

2020-07-22 11:30

developer   ~0124225

Nobody complained about r63584 so far. At least using TProcess should prevent the SIGSEGV which this report is about.
Resolving...

Ondrej Pokorny

2020-08-09 16:53

developer   ~0124696

I do have a complain about r63584. It breaks debugging console applications.

To reproduce create a new empty console application, put a breakpoint somewhere and run debugging. The console window won't show up. Before r63584 the console showed up, which was correct.

Marco van de Voort

2020-08-09 18:15

manager   ~0124699

If said tprocess usage uses ponoconsole it is probably related to

https://wiki.freepascal.org/User_Changes_3.2.0#pnoconsole_on_Windows

In such case, tprocess got ponoconsole, but still console was used. This got rectified, but with RC1 somebody had problems with mysql's console app because of it. For that podetached was created (windows only).

So probably change it to {$ifdef mswindows}podetached{$else}ponoconsole{$endif} or so.

Ondrej Pokorny

2020-08-09 18:20

developer   ~0124700

Yeah, I am working on that.

Ondrej Pokorny

2020-08-09 18:25

developer   ~0124701

I fixed it. FPC that does not declare poNoConsole still needs to use a hack, but now a better one that should work with O4 as well. Please test.

bald zhang

2020-08-24 03:31

reporter   ~0125100

Last edited: 2020-08-24 04:42

View 4 revisions

Compile 2.0.10 with fpc 3.2.1(r46659)
got following messages,
lazarus trunk(r63798) is ok.

UPDATE: 2.0.11(r63819) also crash
C:/fpc/3.2.1/bin/x86_64-win64/gmkdir.exe -p ../units/x86_64-win64/win32
../tools/svn2revisioninc.exe .. revision.inc
An unhandled exception occurred at $000000010000A13A:
EAccessViolation: Access violation
  $000000010000A13A fpc_ansistr_decr_ref, line 146 of ../inc/astrings.inc
  $0000000100012921 fpc_finalize, line 234 of ../inc/rtti.inc
  $0000000100012657 RECORDRTTI, line 134 of ../inc/rtti.inc
  $0000000100010C91 CLEANUPINSTANCE, line 755 of ../inc/objpas.inc
  $0000000100010489 FREEINSTANCE, line 446 of ../inc/objpas.inc
  $000000010004529A DESTROY, line 277 of fcl-process/src/processbody.inc
  $0000000100001962 fin$00000012, line 118 of svn2revisioninc.pas
  $00000001000019ED SVNINPATH, line 109 of svn2revisioninc.pas
  $0000000100003C91 PARAMSVALID, line 572 of svn2revisioninc.pas
  $00000001000046DD GITREVISIONFROMGITCOMMIT, line 769 of svn2revisioninc.pas
  $00000001000047C7 main, line 788 of svn2revisioninc.pas
  $00000001000047E6 MAIN_WRAPPER, line 126 of system.pp
  $000000010001AE27 EXE_ENTRY, line 240 of system.pp
  $000000010000192D _FPC_MAINCRTSTARTUP, line 106 of sysinit.pp
  $00007FFB4D7B7BD4
  $00007FFB4E96CE51

make[1]: *** [revisioninc] Error 217
make[1]: Leaving directory `C:/lazarus-2.0.10/ide'
make: *** [idebig] Error 2

Do-wan Kim

2020-08-24 12:04

reporter   ~0125101

How about with this patch?
35991_svn2revisioninc.pas.patch (1,071 bytes)   
Index: tools/svn2revisioninc.pas
===================================================================
--- tools/svn2revisioninc.pas	(revision 63817)
+++ tools/svn2revisioninc.pas	(working copy)
@@ -281,12 +281,25 @@
     SvnVersionProcess: TProcessUTF8;
     Buffer: string;
     n: LongInt;
+    {$ifdef windows}
+    svnpath : string;
+    {$endif}
   begin
     Result:=false;
     SvnVersionProcess := TProcessUTF8.Create(nil);
     try
       with SvnVersionProcess do begin
+      {$ifdef Windows}
+        svnpath := GetEnvironmentVariableUTF8('FPC_SVN_PATH')+PathDelim+'svnversion';
+        if not FileExistsUTF8(svnpath+'.exe') then begin
+          svnpath := '..\..\fpcbootstrap\svn\bin\svnversion';
+            if not FileExistsUTF8(svnpath+'.exe') then
+               svnpath := 'svnversion';
+        end;
+        CommandLine := svnpath+' -n "' + SourceDirectory + '"';
+      {$else}
         CommandLine := 'svnversion -n "' + SourceDirectory + '"';
+      {$endif}
         Options := [poUsePipes, poWaitOnExit];
         try
           Execute;

bald zhang

2020-08-30 01:28

reporter   ~0125212

@Ondrej Pokorny
HI, I tested fpc 3.2.1 r46718 and lazarus 2.0.11 r63742, this issue still there
Is there any plan to fix it with this bundle?
Or lazarus 2.0.x will stick on fpc 3.2.0?

Marco van de Voort

2020-09-12 21:30

manager   ~0125516

The fix was done to lazarus trunk, it might not be merged to lazarus fixes yet.

Issue History

Date Modified Username Field Change
2019-08-21 00:13 Martin Friebe New Issue
2019-08-21 00:14 Martin Friebe Description Updated View Revisions
2019-08-21 00:14 Martin Friebe LazTarget => -
2019-08-21 00:14 Martin Friebe Widgetset Win32/Win64 => Win32/Win64
2019-08-21 00:19 Martin Friebe Note Added: 0117754
2019-08-21 00:19 Martin Friebe Relationship added related to 0028991
2020-07-14 12:49 Marco van de Voort Note Added: 0124003
2020-07-14 13:51 Bart Broersma Note Added: 0124006
2020-07-14 13:55 Bart Broersma Note Edited: 0124006 View Revisions
2020-07-14 16:15 Sven Barth Note Added: 0124011
2020-07-14 23:38 Juha Manninen Note Added: 0124020
2020-07-14 23:40 Juha Manninen Note Edited: 0124020 View Revisions
2020-07-15 00:54 Martin Friebe Note Added: 0124022
2020-07-15 11:26 Bart Broersma Note Added: 0124034
2020-07-15 22:51 Bart Broersma Note Edited: 0124034 View Revisions
2020-07-15 22:52 Marco van de Voort Note Added: 0124068
2020-07-16 12:10 Juha Manninen Note Added: 0124083
2020-07-16 12:10 Juha Manninen File Added: 0001-LazUtils-Don-t-override-TProcess.Execute-for-TProces.patch
2020-07-16 22:44 Bart Broersma Note Added: 0124103
2020-07-16 23:16 Bart Broersma Note Added: 0124105
2020-07-17 10:13 Juha Manninen Note Added: 0124115
2020-07-17 10:14 Juha Manninen Note Edited: 0124115 View Revisions
2020-07-17 10:14 Juha Manninen Note Edited: 0124115 View Revisions
2020-07-17 15:39 Juha Manninen Note Edited: 0124115 View Revisions
2020-07-17 16:25 Sven Barth Note Added: 0124124
2020-07-18 16:47 Marco van de Voort Note Added: 0124145
2020-07-18 16:47 Marco van de Voort Note Edited: 0124145 View Revisions
2020-07-20 07:38 Sven Barth Note Added: 0124182
2020-07-20 11:28 Bart Broersma Note Added: 0124186
2020-07-20 12:28 Marco van de Voort Note Added: 0124187
2020-07-20 12:35 Marco van de Voort Note Edited: 0124187 View Revisions
2020-07-22 11:30 Juha Manninen Assigned To => Juha Manninen
2020-07-22 11:30 Juha Manninen Status new => resolved
2020-07-22 11:30 Juha Manninen Resolution open => fixed
2020-07-22 11:30 Juha Manninen Fixed in Revision => r63584
2020-07-22 11:30 Juha Manninen Widgetset Win32/Win64 => Win32/Win64
2020-07-22 11:30 Juha Manninen Note Added: 0124225
2020-08-09 16:53 Ondrej Pokorny Status resolved => assigned
2020-08-09 16:53 Ondrej Pokorny Resolution fixed => open
2020-08-09 16:53 Ondrej Pokorny Note Added: 0124696
2020-08-09 17:56 Ondrej Pokorny Relationship added related to 0032055
2020-08-09 18:15 Marco van de Voort Note Added: 0124699
2020-08-09 18:20 Ondrej Pokorny Note Added: 0124700
2020-08-09 18:20 Ondrej Pokorny Assigned To Juha Manninen => Ondrej Pokorny
2020-08-09 18:25 Ondrej Pokorny Status assigned => resolved
2020-08-09 18:25 Ondrej Pokorny Resolution open => fixed
2020-08-09 18:25 Ondrej Pokorny Fixed in Revision r63584 => r63584, r63708
2020-08-09 18:25 Ondrej Pokorny Widgetset Win32/Win64 => Win32/Win64
2020-08-09 18:25 Ondrej Pokorny Note Added: 0124701
2020-08-24 03:31 bald zhang Note Added: 0125100
2020-08-24 03:41 bald zhang Note Edited: 0125100 View Revisions
2020-08-24 03:42 bald zhang Note Edited: 0125100 View Revisions
2020-08-24 04:42 bald zhang Note Edited: 0125100 View Revisions
2020-08-24 12:04 Do-wan Kim Note Added: 0125101
2020-08-24 12:04 Do-wan Kim File Added: 35991_svn2revisioninc.pas.patch
2020-08-30 01:28 bald zhang Note Added: 0125212
2020-09-12 21:30 Marco van de Voort Note Added: 0125516