View Issue Details

IDProjectCategoryView StatusLast Update
0037133LazarusIDEpublic2020-07-13 17:49
ReporterCyrax Assigned ToJuha Manninen  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
PlatformLinux x86_64OSArch 
Product Version2.1 (SVN) 
Summary0037133: Running lazbuild through valgrind shows that a empty string is passed to SysUtils.DIRECTORYEXISTS function.
DescriptionI have noticed random crashes at lazbuild when trying to build same project to different targets.
Trying to fix these crashes by using valgrind, it is have notified me that there is something odd when calling DirectoryExists function.

See Additional Information field for more info.
Additional Information
==63588== Syscall param stat(file_name) points to unaddressable byte(s)
==63588==    at 0x40D104: SYSTEM_$$_FPSYSCALL$INT64$INT64$INT64$$INT64 (syscall.inc:80)
==63588==    by 0x40DFAE: SYSTEM_$$_FPSTAT$PCHAR$STAT$$LONGINT (ossysc.inc:121)
==63588==    by 0x4C0479: SYSUTILS_$$_DIRECTORYEXISTS$RAWBYTESTRING$BOOLEAN$$BOOLEAN (sysutils.pp:693)
==63588==    by 0x4DD47E: LAZFILEUTILS_$$_DIRECTORYEXISTSUTF8$ANSISTRING$$BOOLEAN (unixlazfileutils.inc:47)
==63588==    by 0x4E03E9: LAZFILEUTILS_$$_DIRPATHEXISTS$ANSISTRING$$BOOLEAN (lazfileutils.pas:520)
==63588==    by 0x4E067F: LAZFILEUTILS_$$_FORCEDIRECTORY$ANSISTRING$$BOOLEAN (lazfileutils.pas:552)
==63588==    by 0x405B3E: P$LAZBUILD$_$TLAZBUILDAPPLICATION_$_BUILDPROJECT$ANSISTRING$$BOOLEAN_$$_STARTBUILDING$$BOOLEAN (lazbuild.lpr:836)
==63588==    by 0x404EB3: P$LAZBUILD$_$TLAZBUILDAPPLICATION_$__$$_BUILDPROJECT$ANSISTRING$$BOOLEAN (lazbuild.lpr:937)
==63588==    by 0x402C2C: P$LAZBUILD$_$TLAZBUILDAPPLICATION_$__$$_BUILDFILE$ANSISTRING$$BOOLEAN (lazbuild.lpr:426)
==63588==    by 0x409E00: P$LAZBUILD$_$TLAZBUILDAPPLICATION_$__$$_RUN (lazbuild.lpr:1475)
==63588==    by 0x40D065: main (lazbuild.lpr:1878)
==63588==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
TagsNo tags attached.
Fixed in Revisionr63327
LazTarget-
WidgetsetGTK 2
Attached Files

Activities

Cyrax

2020-05-23 20:32

reporter   ~0123016

Running valgrind throught vgdb proxy and using it as remote target for GDB debugger running under the IDE and the project , Lazarus have stopped at SIGTRAP.

Call stack follows :
#0 FPSYSCALL(0, 137422174456, 137422174456) at x86_64/syscall.inc:80
0000001 FPSTAT(0x0, {ST_DEV = 6148914691236517205, ST_INO = 6148914691236517205, ST_NLINK = 6148914691236517205, ST_MODE = 1431655765, ST_UID = 1431655765, ST_GID = 1431655765, __PAD1 = 1431655765, ST_RDEV = 6148914691236517205, ST_SIZE = 6148914691236517205, ST_BLKSIZE = 6148914691236517205, ST_BLOCKS = 6148914691236517205, ST_ATIME = 6148914691236517205, ST_ATIME_NSEC = 6148914691236517205, ST_MTIME = 6148914691236517205, ST_MTIME_NSEC = 6148914691236517205, ST_CTIME = 6148914691236517205, ST_CTIME_NSEC = 6148914691236517205, __UNUSED2 = {6148914691236517205, 6148914691236517205, 6148914691236517205}}) at ossysc.inc:121
0000002 DIRECTORYEXISTS(0x0, true) at ../unix/sysutils.pp:693
0000003 DIRECTORYEXISTSUTF8(0x0) at unixlazfileutils.inc:47
0000004 DIRPATHEXISTS(0x0) at lazfileutils.pas:520
0000005 FORCEDIRECTORY(0x7340b90 '/mnt/shares/ohjelmointi/3'...) at lazfileutils.pas:552
0000006 STARTBUILDING(0x1ffeffffa0) at lazbuild.lpr:836
0000007 BUILDPROJECT(0x4c49f08, 0x4c54c90 '/mnt/shares/ohjelmointi/3'...) at lazbuild.lpr:937
0000008 BUILDFILE(0x4c49f08, 0x4c54c90 '/mnt/shares/ohjelmointi/3'...) at lazbuild.lpr:426
0000009 RUN(0x4c49f08) at lazbuild.lpr:1475
0000010 main at lazbuild.lpr:1878

Martok

2020-05-23 20:34

reporter   ~0123017

Last edited: 2020-05-23 20:38

View 2 revisions

Could this be related to 0037034? Some string operations appear to be broken.

Cyrax

2020-05-23 20:43

reporter   ~0123019

Last edited: 2020-05-23 20:48

View 2 revisions

Hmm, I don't know. It seems that Windows target is broken, not others.

See this code snippet from lazfileutils.pas unit:
function ForceDirectory(DirectoryName: string): boolean;
var i: integer;
  Dir: string;
begin
  DirectoryName:=AppendPathDelim(DirectoryName);
  i:=1;
  while i<=length(DirectoryName) do begin
    if DirectoryName[i] in AllowDirectorySeparators then begin
      Dir:=copy(DirectoryName,1,i-1);
      if not DirPathExists(Dir) then begin
        Result:=CreateDirUTF8(Dir);
        if not Result then exit;
      end;
    end;
    inc(i);
  end;
  Result:=true;
end;


At beginning loop and variable i is 1, this clause (copy) will return empty string.
Dir:=copy(DirectoryName,1,i-1);


This will trigger valgrind.
If I remove -1 expression off, valgrind is happy and will continue execution.
There should be check for empty string or just remove the -1.

Bart Broersma

2020-05-24 10:47

developer   ~0123028

Last edited: 2020-05-24 10:48

View 2 revisions

Adding some debug statments shows (Windows):
Dir="", DirPathExists(Dir)=FALSE
CreateDirUTF8("")=TRUE
Dir="\mnt", DirPathExists(Dir)=FALSE
CreateDirUTF8("\mnt")=TRUE
Dir="\mnt\shares", DirPathExists(Dir)=FALSE
CreateDirUTF8("\mnt\shares")=TRUE
Dir="\mnt\shares\ohjelmointi", DirPathExists(Dir)=FALSE
CreateDirUTF8("\mnt\shares\ohjelmointi")=TRUE
Dir="\mnt\shares\ohjelmointi\3", DirPathExists(Dir)=FALSE
CreateDirUTF8("\mnt\shares\ohjelmointi\3")=TRUE

CreateDir(EmptyStr) returns True, so this should not really matter.

You can create a patch to optimize this.
Notice however that CreateDirectory(EmptyStr) should still return True.

Cyrax

2020-06-06 04:02

reporter   ~0123251

lazfileutils.pas.diff (544 bytes)   
diff --git a/components/lazutils/lazfileutils.pas b/components/lazutils/lazfileutils.pas
index caebaf67b1..1e175f819d 100644
--- a/components/lazutils/lazfileutils.pas
+++ b/components/lazutils/lazfileutils.pas
@@ -548,7 +548,7 @@ begin
   i:=1;
   while i<=length(DirectoryName) do begin
     if DirectoryName[i] in AllowDirectorySeparators then begin
-      Dir:=copy(DirectoryName,1,i-1);
+      Dir:=copy(DirectoryName,1,i);
       if not DirPathExists(Dir) then begin
         Result:=CreateDirUTF8(Dir);
         if not Result then exit;
lazfileutils.pas.diff (544 bytes)   

Bart Broersma

2020-06-06 11:23

developer   ~0123258

AFAICS there is no relationship with 0037034, which only happens with fpoc trunk and is about the compiler failing to concatenate 3 strings together.
This issue is about how ForceDirectory is coded.

Juha Manninen

2020-06-06 11:38

developer   ~0123260

Last edited: 2020-06-06 11:41

View 3 revisions

Cyrax, the "-1" leaves out the DirectorySeparator. I think it is correct.
It means the DirPathExists() can be replaced with DirectoryExistsUTF8().
DirPathExists() only chomps out a DirectorySeparator.

If I understand right, an empty string happens when the DirectoryName starts with a separator, in your case '/mnt/shares/ohjelmointi/3'.
We must check for an empty string then.
Please test with my patch. I only tested it compiles. I can test more later, now busy...
0001-ForceDirectory-fix.patch (866 bytes)   
From 78c64b47252c2e03f832245007e51cee7fbf62d7 Mon Sep 17 00:00:00 2001
From: Juha <juha.manninen62@gmail.com>
Date: Sat, 6 Jun 2020 12:27:05 +0300
Subject: [PATCH] ForceDirectory fix.

---
 components/lazutils/lazfileutils.pas | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/components/lazutils/lazfileutils.pas b/components/lazutils/lazfileutils.pas
index caebaf67b..cd2ba3cb3 100644
--- a/components/lazutils/lazfileutils.pas
+++ b/components/lazutils/lazfileutils.pas
@@ -549,7 +549,7 @@ begin
   while i<=length(DirectoryName) do begin
     if DirectoryName[i] in AllowDirectorySeparators then begin
       Dir:=copy(DirectoryName,1,i-1);
-      if not DirPathExists(Dir) then begin
+      if (Dir<>'') and not DirectoryExistsUTF8(Dir) then begin
         Result:=CreateDirUTF8(Dir);
         if not Result then exit;
       end;
-- 
2.26.2

Bart Broersma

2020-06-06 14:20

developer   ~0123261

> DirPathExists() only chomps out a DirectorySeparator.
It will do a little more than that, it also correctly handles UNC filenames, so better not replace that.
Just adding a simple check for (Dir >'') would suffice IMO.
(Dir>'' should result in slightly better asm code than Dir<>'', but that's a micro-optimization)

Juha Manninen

2020-06-06 14:45

developer   ~0123266

Last edited: 2020-06-06 15:12

View 3 revisions

Bart, you are right about DirPathExists(). It calls ChompPathDelim() which handles UNC filenames.
However now ForceDirectory() does not handle UNC filenames even if DirPathExists() is called, or even with the patch from Cyrax.
Should it?

What does (Dir >'') do? I am puzzled. I should know Pascal and I thought ">" is an arithmetic operator. But yes, it compiles.

Bart Broersma

2020-06-07 21:55

developer   ~0123316

Last edited: 2020-06-07 22:03

View 2 revisions

Juha: I don't know if ForceDirectory is supposed to handle UNC paths.
If it doesn't now, I would just document that.
Dir > '' just compares Dir alphabetically to an empty string, just like you can check wether 'abc' > 'ABC' (evaluates to FALSE IIRC).

For now I would say that adding a check for Dir<> '' is enough. (Dir<>'' is more self-explanatory then Dir>'' ...)
More optimizations could be done, but the bottle neck will be the IO calls always.

Bart Broersma

2020-06-07 22:13

developer   ~0123318

Another optimization:

function ForceDirectory(DirectoryName: string): boolean;
var i: integer;
  Dir: string;
begin
  DirectoryName:=AppendPathDelim(DirectoryName);
  i:=1;
  while i<=length(DirectoryName) do begin
    if DirectoryName[i] in AllowDirectorySeparators then begin
      // paths like \foo\\bar\\foobar
      while (i<length(DirectoryName)) and (DirectoryName[i+1] in AllowDirectorySeparators) do System.Delete(DirectoryName,i+1,1);
      Dir:=copy(DirectoryName,1,i-1);
      if (Dir > '') and not DirPathExists(Dir) then begin
        Result:=CreateDirUTF8(Dir);
        if not Result then exit;
      end;
    end;
    inc(i);
  end;
  Result:=true;
end;

This prevents several unneccessary calls to DirPathExists.
Windows API accept filenames like \\foo\\bar, they just throw away the double backslashes.

Juha Manninen

2020-06-08 12:36

developer   ~0123335

I committed Bart's idea. DirectoryExists() should not be called with an empty string any more.
Please test.

Cyrax

2020-07-13 17:49

reporter   ~0123986

Thanks for the fix!

Issue History

Date Modified Username Field Change
2020-05-23 20:28 Cyrax New Issue
2020-05-23 20:32 Cyrax Note Added: 0123016
2020-05-23 20:34 Martok Note Added: 0123017
2020-05-23 20:38 Martok Note Edited: 0123017 View Revisions
2020-05-23 20:43 Cyrax Note Added: 0123019
2020-05-23 20:48 Cyrax Note Edited: 0123019 View Revisions
2020-05-24 10:47 Bart Broersma Note Added: 0123028
2020-05-24 10:48 Bart Broersma Note Edited: 0123028 View Revisions
2020-06-06 04:02 Cyrax Note Added: 0123251
2020-06-06 04:02 Cyrax File Added: lazfileutils.pas.diff
2020-06-06 11:06 Juha Manninen Relationship added related to 0037034
2020-06-06 11:23 Bart Broersma Note Added: 0123258
2020-06-06 11:23 Bart Broersma Relationship deleted related to 0037034
2020-06-06 11:38 Juha Manninen Note Added: 0123260
2020-06-06 11:38 Juha Manninen File Added: 0001-ForceDirectory-fix.patch
2020-06-06 11:39 Juha Manninen Note Edited: 0123260 View Revisions
2020-06-06 11:41 Juha Manninen Note Edited: 0123260 View Revisions
2020-06-06 11:41 Juha Manninen Assigned To => Juha Manninen
2020-06-06 11:41 Juha Manninen Status new => feedback
2020-06-06 11:41 Juha Manninen LazTarget => -
2020-06-06 14:20 Bart Broersma Note Added: 0123261
2020-06-06 14:45 Juha Manninen Note Added: 0123266
2020-06-06 14:58 Juha Manninen Note Edited: 0123266 View Revisions
2020-06-06 15:12 Juha Manninen Note Edited: 0123266 View Revisions
2020-06-07 21:55 Bart Broersma Note Added: 0123316
2020-06-07 22:03 Bart Broersma Note Edited: 0123316 View Revisions
2020-06-07 22:13 Bart Broersma Note Added: 0123318
2020-06-08 12:36 Juha Manninen Status feedback => resolved
2020-06-08 12:36 Juha Manninen Resolution open => fixed
2020-06-08 12:36 Juha Manninen Fixed in Revision => r63327
2020-06-08 12:36 Juha Manninen Widgetset GTK 2 => GTK 2
2020-06-08 12:36 Juha Manninen Note Added: 0123335
2020-07-13 17:49 Cyrax Status resolved => closed
2020-07-13 17:49 Cyrax Note Added: 0123986