View Issue Details

IDProjectCategoryView StatusLast Update
0034500FPCRTLpublic2019-01-27 18:09
ReporterTheo van OostenAssigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
PlatformLinuxOSMintOS Version19
Product Version3.0.4Product Build 
Target Version3.2.0Fixed in Version3.3.1 
Summary0034500: Application crashes in unit CRT when executing a writeln
DescriptionTo investigate why text is always wrapped at 80 char's, I copied the file CRT.PP (including CRTH.INC) to my local folder. From that moment my application crashed at the very first writeln. I traced the problem to the procedure CRTWRITE at line 1266 in the unit CRT. There a memory copy is performed (MOVE), followed by SETLENGTH to adjust the size of the receiving string; This should be reversed. SETLENGTH allocates the memory needed for the larger string; MOVE does not look if the memory is allocated, so if not: SIGSEGV!
Steps To ReproduceCopy CRT.PP and CRTH.INC to a local folder. Create a new project, console application, and put crt in the uses clause. In project options, add this local folder (where you copied CRT.PP to) to compilation paths. Put a writeln in your project and press F9.
Additional InformationTo confirm and solve the problem, I renamed CRT.PP to MYCRT (also the unit clause inside the file and the uses clause in the project file). When the crash was confirmed, I put the setlength statement on line 1267 one line higher (before the move statement). No more crashes... I just wonder why there are no crashes when using CRT on its original location??
I did all this because a max line length of 80 is not suitable for me. The fpIOCtl call in GetConsoleBuf returns a width and height of 0 (only in Linux?). So the actual console size is not reported and set to 80 x 25. Can you please add a procedure "set_screen_size", or something like that? I will add that to MYCRT because I really need it.
TagsNo tags attached.
Fixed in Revision40912
FPCOldBugId
FPCTarget
Attached Files

Activities

Theo van Oosten

2018-11-03 17:47

reporter   ~0111757

I added the following procedure:

Procedure SetScreenSize (width, height: integer);

begin
  if width > ConsoleMaxX then width := ConsoleMaxX;
  if height > ConsoleMaxY then height := ConsoleMaxY;
  if Assigned(ConsoleBuf) then
    FreeMem(ConsoleBuf,ScreenHeight*ScreenWidth*2);
  ScreenWidth:=width;
  ScreenHeight:=height;
  WindMaxX:=width;
  WindMaxY:=height;
  GetMem(ConsoleBuf,ScreenHeight*ScreenWidth*2);
  FillChar(ConsoleBuf^,ScreenHeight*ScreenWidth*2,0);
end;

Now you only have to cut and paste to add the routine.

Florian

2018-11-03 18:35

administrator   ~0111758

> SETLENGTH allocates the memory needed for the larger string

The CRT unit is not made to be compiled using objfpc or ansistrings. For shortstrings, the used approach is perfectly fine.

Michael Van Canneyt

2018-11-04 13:14

administrator   ~0111783

1/ Allowing you to set the screen size is not the solution. We need to know why the current size is not detected properly.

2/ As Florian remarked, the unit is meant to be used with shortstring, so the setlength after move is OK. I will however add some directives to force the correct string type.

Can you please provide an example stack trace showing that the screen size is 0 ?
If it were so, none of the term based programs (most if not all editors) would work. Do they work in the same terminal ?

Here the ioctl always gives a screen size, for example:
ioctl(1, TIOCGWINSZ, {ws_row=50, ws_col=132, ws_xpixel=0, ws_ypixel=0}) = 0

Theo van Oosten

2018-11-06 12:50

reporter   ~0111809

Last edited: 2018-11-06 12:55

View 2 revisions

1/ agreed

2/ If it has to be a shortstring, then define it i.e: var Temp : String [255];

FPioctl is used, there is no posibility to use ioctl, I looked for it.
I do not know how to get a stacktrace in a normal running program, so I uploaded a picture of the debugsession.
All other programs work fine, this problem with Lazarus has been there since I started using Lazarus, version 0.9.....

Theo van Oosten

2018-11-06 12:54

reporter  

20181105_095612-2.jpg (373,119 bytes)

Marco van de Voort

2018-11-08 12:27

manager   ~0111861

Try to strace it (and preferably using the original, release crt) and see if you can see the ioctl results there.

Theo van Oosten

2018-11-10 13:19

reporter   ~0111889

Last edited: 2018-11-10 13:43

View 2 revisions

I created a simple program (Hello world), added crt to the uses clause and straced it. The output contained the following line:
ioctl(1, TIOCGWINSZ, {ws_row=49, ws_col=180, ws_xpixel=0, ws_ypixel=0}) = 0
But it wouldn't let me put a breakpoint in GetConsoleBuf;
So I replaced crt by mycrt and repeated. Same result. But now I could put a breakpoint in GetConsoleBuf and check the contents of Wininfo: all zero's; So I suspect the problem is in fpIOCtl.

Thaddy de Koning

2018-11-10 14:23

reporter   ~0111890

FPioctl is an fpc alias for ioctl. It is the exact same.

Theo van Oosten

2018-11-10 19:22

reporter   ~0111894

How do you explain that strace reports valid values and FPioctl reports zero's?

Thaddy de Koning

2018-11-10 20:56

reporter   ~0111895

Last edited: 2018-11-10 21:00

View 2 revisions

function real_FpIOCtl (Handle:cint;Ndx: TIOCtlRequest):cint; cdecl; varargs; external clib name 'ioctl';

function FpIOCtl (Handle:cint;Ndx: TIOCtlRequest;Data: Pointer):cint;
begin
  FpIOCtl:=real_FpIOCtl(Handle, Ndx, Data);
end;

Technically it could possibly also be declared or overloaded as:
function FpIOCtl (Handle:cint;Ndx: TIOCtlRequest;Data:array of const):cint;cdecl;external clib name 'ioctl';
Because FPC handles that correctly (Delphi doesn't know this)

Theo van Oosten

2018-11-11 12:55

reporter   ~0111907

Last edited: 2018-11-11 12:55

View 2 revisions

Copy/pasted your solution, no change. And clib should be 'c'.

Thaddy de Koning

2018-11-11 15:43

reporter  

gotoxy.png (24,502 bytes)
gotoxy.png (24,502 bytes)

Thaddy de Koning

2018-11-11 15:46

reporter   ~0111908

Last edited: 2018-11-11 15:59

View 4 revisions

I added a screenshot that shows that if your terminal settings are ok strings larger than 80 are allowed. There is nothing much I could find otherwise on both windows and linux.
My terminal settings for this test are 132X50 and not from code.
clib is declared as a const and is 'c'.
But you have a point if - and only if - string[80] is in the code. I could not find it.
I really tested this carefully.

Theo van Oosten

2018-11-11 16:54

reporter   ~0111909

When I use my procedure to set the screen size explicitly, everything works fine. Very large strings can be used. That is not the problem.
It's the terminal settings that cannot be determined. If you call IOctl (or FPioctl) strace shows the correct settings being reported, but the parameter in the IOctl call that should receive these settings is set to zero. Somewhere in the chain of call's the information is lost. I followed the flow in assembly and I cannot see where it goes wrong. I have not much time now, but as soon as I see a chance I will try to look deeper into the assembly code and try to copy it to a debugable unit. I am looking forward to that. It 's a long time ago I did something in assembly.

Thaddy de Koning

2018-11-11 21:43

reporter   ~0111911

You don't need to to go to assembler. The bug is in your pascal code. Can you send it to me in private?

Theo van Oosten

2018-11-12 07:15

reporter   ~0111920

Last edited: 2018-11-12 07:18

View 3 revisions

It is not in MY pascal code. The problem is in CRT.PP
(/usr/share/fpcsrc/3.0.4/packages/rtl-control/src/unix/crt.pp)


Procedure GetConsoleBuf;
var
  WinInfo : TWinSize;
begin
  if Assigned(ConsoleBuf) then
    FreeMem(ConsoleBuf,ScreenHeight*ScreenWidth*2);
  ScreenWidth:=0;
  ScreenHeight:=0;
  if (not OutputRedir) and (fpIOCtl(TextRec(Output).Handle,TIOCGWINSZ,@Wininfo)>=0) then
    begin
    ScreenWidth:=Wininfo.ws_col;
    ScreenHeight:=Wininfo.ws_row;
    end;
  // Set some arbitrary defaults which make some sense...
  If (ScreenWidth=0) then
     ScreenWidth:=80;
  If (ScreenHeight=0) then
     ScreenHeight:=25;
  GetMem(ConsoleBuf,ScreenHeight*ScreenWidth*2);
  FillChar(ConsoleBuf^,ScreenHeight*ScreenWidth*2,0);
end;

Thaddy de Koning

2018-11-12 08:42

reporter   ~0111922

Last edited: 2018-11-12 08:59

View 5 revisions

Well I reduced it a bit and it tests OK here:
uses
  Baseunix, termio, crt;
var
  WinInfo : TWinSize;

begin
 if fpIOCtl(TextRec(Output).Handle,TIOCGWINSZ,@Wininfo)>=0) then
    writeln(Wininfo.ws_col,'X',Wininfo.ws_row); // in my case 130X50
end.

If this works for you the cause must be (not OutputRedir). If you redirected your output? Problem is I can't bring that out, because it is declared in the implementation section.

Btw this path is wrong /usr/share/fpcsrc/3.0.4/packages/rtl-control
It is /usr/share/fpcsrc/3.0.4/packages/rtl-console

Thaddy de Koning

2018-11-12 09:29

reporter   ~0111924

I have bee able to somewhat reproduce the issue:

I can get this:

130X50
80X25
130X50
Marked memory at $76EF53E0 invalid
Wrong size : 13000 allocated 4000 freed
  $00033A70
Call trace for block $76EF53E0 size 13000

based on:
program demo2;
{$H-}
uses
  Baseunix, unix, termio, crt;
var
  WinInfo : TWinSize;
begin
 writeln(ScreenWidth,'X',ScreenHeight);
 ScreenWidth := 80;
 ScreenHeight := 25;
 writeln(ScreenWidth,'X',ScreenHeight);
 if fpIOCtl(TextRec(Output).Handle,TIOCGWINSZ,@Wininfo)>=0 then
    writeln(Wininfo.ws_col,'X',Wininfo.ws_row);
end.

So if you change the screenwidth/height through its vars it crashes.
That should make debugging easier.

Thaddy de Koning

2018-11-12 10:34

reporter   ~0111928

Last edited: 2018-11-12 11:46

View 3 revisions

[edit: I updated the code!!]
Possible patch that sync actual term w/h and screenwidth/height.

---
Procedure GetConsoleBuf;
var
  WinInfo : TWinSize;
begin
  if Assigned(ConsoleBuf) then
    FreeMem(ConsoleBuf);
  // start with some default
  ScreenWidth := 80;
  ScreenHeight := 25;
  // get true width and height
  if (not OutputRedir) and (fpIOCtl(TextRec(Output).Handle,TIOCGWINSZ,@Wininfo)>=0) then
    begin
    ScreenWidth:=Wininfo.ws_col;
    ScreenHeight:=Wininfo.ws_row;
    end else
    begin
      // Set some arbitrary defaults which make some sense...
      WinInfo.ws_col := ScreenWidth;
      WinInfo.ws_row := ScreenHeight;
      // Now actually set width and height, sync actual terminal width and height
      if (not OutputRedir) and fpIOCtl(TextRec(Output).Handle,TIOCSWINSZ,@Wininfo)>=0 then
      begin
        ScreenWidth := WinInfo.ws_col;
        ScreenHeight := WinInfo.ws_row;
      end;
    end;
  GetMem(ConsoleBuf,ScreenHeight*ScreenWidth*2);
  FillChar(ConsoleBuf^,ScreenHeight*ScreenWidth*2,0);
end;

Theo van Oosten

2018-11-14 10:15

reporter   ~0111966

I tried your reduced solution with no result. Then I wanted to see the strace. Different outcome! It reported window size correctly.
Then I run the program in a terminal. And it showed the correct window size.
I run it again in Lazarus, window size was zero again. The problem is in Lazarus!

Thaddy de Koning

2018-11-14 16:48

reporter   ~0111971

Last edited: 2018-11-14 16:52

View 3 revisions

"And it showed the correct window size."
That's what I saw. I did not use Lazarus, because it was an fpc bug. (at least partially it is as your tests showed as well.)

This was the critical part:
      // Now actually set width and height, sync actual terminal width and height
      if (not OutputRedir) and fpIOCtl(TextRec(Output).Handle,TIOCSWINSZ,@Wininfo)>=0 then

AND
 The Freemem() needs to be without size, because that can be changed at run-time

Thaddy de Koning

2018-11-14 16:56

reporter   ~0111972

I am still not sure my suggestion is 100% correct, but it is more correct.

Theo van Oosten

2018-11-15 16:07

reporter   ~0111985

I do not understand how setting the width and height is the solution while you do not know its values. And the code you've added will only be called if OutputRedir is true or fpIOCtl is not succesful.
In Lazarus the console is one of the windows, opened by Lazarus. IOCtl reports the size of the terminal, which is not present, as all output is redirected to this window. I do not know how to make IOCtl report the size of that window. That's something for the creators of Lazarus.
I will keep using my SetScreenSize procedure if only to force lines being longer then the screen width and let the terminal emulator do the wrapping. That way I can enlarge the terminal window and see the already written lines getting longer as the terminal widens.

Michael Van Canneyt

2018-11-15 16:20

administrator   ~0111986

What options do you set in lazarus when running the program ?

If you don't set any options, then the program will not have a terminal, sizes will be zero, and indeed the program will crash.

Theo van Oosten

2018-11-16 11:41

reporter   ~0111992

Last edited: 2018-11-16 11:59

View 2 revisions

Michael, no options set, just choose "new project", then "programm" (3e option).
How do you catch the output to send it to the console window? Could the same mechanism be used to catch the IOCtl call?

Thaddy de Koning

2018-11-16 15:07

reporter   ~0112002

Last edited: 2018-11-16 15:09

View 2 revisions

Michael,
When you go through the code of the crt unit, you will notice that a lot of assumptions are made about screenwidth and screenhight being correct.
The crt unit does so without any evaluation at runtime.
If the - current - call to set w/h fails, there's a default that can possibly be wrong. What I did is an attempt to keep the real values, from the OS, in sync with what the rest of the code expects. Current code does not do that. The testing by Theo https://bugs.freepascal.org/view.php?id=34500#c111966 seems to suggest I am on the right track but I am not sure.

Thaddy de Koning

2018-11-16 15:10

reporter   ~0112003

Also note TIOCGWINSZ does not equal TIOCSWINSZ

Marco van de Voort

2018-11-18 15:31

manager   ~0112046

Theo, what architecture are the generated binaries, I assume x86, but 32-bit or 64-bit ?

Theo van Oosten

2018-11-21 12:02

reporter   ~0112100

Marco: 64 bit.

Michael Van Canneyt

2019-01-19 17:57

administrator   ~0113489

I added a mode statement to make sure shortstrings are used.

For the terminal size:

If the terminal size cannot be detected (such as when output is redirected) then there is nothing that can be done. If Lazarus redirects the terminal, then it simply means crt cannot be debugged correctly in lazarus. (for example CGI programs also cannot be easily debugged). You can always run your program in a terminal and let Lazarus attach to it.

The default setting in case no terminal is detected is then 80x25.

I don't think setting it to a different value makes any sense, if the size cannot be detected, something is wrong in the environment and that should be fixed instead.

Theo van Oosten

2019-01-27 18:09

reporter   ~0113676

I disagree with the choosen solution. A mode statement to make sure shortstrings are used can easy be overlooked, even by experienced programmers. It would be more consistent to define a short string in stead of forcing a long string to a short string by a mode statement.

Issue History

Date Modified Username Field Change
2018-11-02 17:02 Theo van Oosten New Issue
2018-11-03 17:47 Theo van Oosten Note Added: 0111757
2018-11-03 18:35 Florian Note Added: 0111758
2018-11-04 13:09 Michael Van Canneyt Assigned To => Michael Van Canneyt
2018-11-04 13:09 Michael Van Canneyt Status new => assigned
2018-11-04 13:14 Michael Van Canneyt Note Added: 0111783
2018-11-04 13:14 Michael Van Canneyt Status assigned => feedback
2018-11-06 12:50 Theo van Oosten Note Added: 0111809
2018-11-06 12:50 Theo van Oosten Status feedback => assigned
2018-11-06 12:54 Theo van Oosten File Added: 20181105_095612-2.jpg
2018-11-06 12:55 Theo van Oosten Note Edited: 0111809 View Revisions
2018-11-08 12:27 Marco van de Voort Note Added: 0111861
2018-11-10 13:19 Theo van Oosten Note Added: 0111889
2018-11-10 13:43 Theo van Oosten Note Edited: 0111889 View Revisions
2018-11-10 14:23 Thaddy de Koning Note Added: 0111890
2018-11-10 19:22 Theo van Oosten Note Added: 0111894
2018-11-10 20:56 Thaddy de Koning Note Added: 0111895
2018-11-10 21:00 Thaddy de Koning Note Edited: 0111895 View Revisions
2018-11-11 12:55 Theo van Oosten Note Added: 0111907
2018-11-11 12:55 Theo van Oosten Note Edited: 0111907 View Revisions
2018-11-11 15:43 Thaddy de Koning File Added: gotoxy.png
2018-11-11 15:46 Thaddy de Koning Note Added: 0111908
2018-11-11 15:47 Thaddy de Koning Note Edited: 0111908 View Revisions
2018-11-11 15:57 Thaddy de Koning Note Edited: 0111908 View Revisions
2018-11-11 15:59 Thaddy de Koning Note Edited: 0111908 View Revisions
2018-11-11 16:54 Theo van Oosten Note Added: 0111909
2018-11-11 21:43 Thaddy de Koning Note Added: 0111911
2018-11-12 07:15 Theo van Oosten Note Added: 0111920
2018-11-12 07:16 Theo van Oosten Note Edited: 0111920 View Revisions
2018-11-12 07:18 Theo van Oosten Note Edited: 0111920 View Revisions
2018-11-12 08:42 Thaddy de Koning Note Added: 0111922
2018-11-12 08:44 Thaddy de Koning Note Edited: 0111922 View Revisions
2018-11-12 08:49 Thaddy de Koning Note Edited: 0111922 View Revisions
2018-11-12 08:58 Thaddy de Koning Note Edited: 0111922 View Revisions
2018-11-12 08:59 Thaddy de Koning Note Edited: 0111922 View Revisions
2018-11-12 09:29 Thaddy de Koning Note Added: 0111924
2018-11-12 10:34 Thaddy de Koning Note Added: 0111928
2018-11-12 10:36 Thaddy de Koning Note Edited: 0111928 View Revisions
2018-11-12 11:46 Thaddy de Koning Note Edited: 0111928 View Revisions
2018-11-14 10:15 Theo van Oosten Note Added: 0111966
2018-11-14 16:48 Thaddy de Koning Note Added: 0111971
2018-11-14 16:49 Thaddy de Koning Note Edited: 0111971 View Revisions
2018-11-14 16:52 Thaddy de Koning Note Edited: 0111971 View Revisions
2018-11-14 16:56 Thaddy de Koning Note Added: 0111972
2018-11-15 16:07 Theo van Oosten Note Added: 0111985
2018-11-15 16:20 Michael Van Canneyt Note Added: 0111986
2018-11-16 11:41 Theo van Oosten Note Added: 0111992
2018-11-16 11:59 Theo van Oosten Note Edited: 0111992 View Revisions
2018-11-16 15:07 Thaddy de Koning Note Added: 0112002
2018-11-16 15:09 Thaddy de Koning Note Edited: 0112002 View Revisions
2018-11-16 15:10 Thaddy de Koning Note Added: 0112003
2018-11-18 15:31 Marco van de Voort Note Added: 0112046
2018-11-21 12:02 Theo van Oosten Note Added: 0112100
2019-01-19 17:57 Michael Van Canneyt Fixed in Revision => 40912
2019-01-19 17:57 Michael Van Canneyt Note Added: 0113489
2019-01-19 17:57 Michael Van Canneyt Status assigned => resolved
2019-01-19 17:57 Michael Van Canneyt Fixed in Version => 3.3.1
2019-01-19 17:57 Michael Van Canneyt Resolution open => fixed
2019-01-19 17:57 Michael Van Canneyt Target Version => 3.2.0
2019-01-27 18:09 Theo van Oosten Note Added: 0113676
2019-01-27 18:09 Theo van Oosten Status resolved => closed