View Issue Details

IDProjectCategoryView StatusLast Update
0030331FPCRTLpublic2016-07-11 16:44
ReporterSeth GroverAssigned ToMarco van de Voort 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Platformx86_64OSLinuxOS VersionDebian 8
Product Version3.0.0Product Build3.0 built from svn rev 33098 
Target VersionFixed in Version3.0.2 
Summary0030331: fd file descriptor in Fpopendir in rtl/linux/ossysc.inc overflows if > 32*1024
DescriptionSee this thread:

http://lists.freepascal.org/pipermail/fpc-devel/2016-June/037189.html

I am debugging an issue with FPC 3.0 on Linux x86_64 which I believe I have
tracked down to Fpopendir rtl/linux/ossysc.inc:

-----------------------------------------------------------
function Fpopendir(dirname : pchar): pdir; [public, alias :
'FPC_SYSC_OPENDIR'];

var
  fd:integer;
  st:stat;
  ptr:pdir;

begin
  Fpopendir:=nil;
  if Fpstat(dirname,st)<0 then
   exit;
{ Is it a dir ? }
  if not((st.st_mode and $f000)=$4000)then
   begin
     errno:=ESysENOTDIR;
     exit
   end;
{ Open it}
  fd:=Fpopen(dirname,O_RDONLY,438);
  if fd<0 then
   exit;
  new(ptr);
  if ptr=nil then
   exit;
  new(ptr^.dd_buf);
  if ptr^.dd_buf=nil then
   exit;
  ptr^.dd_fd:=fd;
  ptr^.dd_loc:=0;
  ptr^.dd_size:=0;
  ptr^.dd_nextoff:=0;
  ptr^.dd_max:=sizeof(ptr^.dd_buf^);
  Fpopendir:=ptr;
end;
-----------------------------------------------------------

In this process I have a LOT of open file handles. The system on which it
runs uses "ulimit -n 64000" to set the maximum number of open descriptors
to 64,000. However, once I get above 32768 file handles Fpopendir starts
failing, and causing me to leak directory descriptors. I didn't have debug
compiled down into the RTL, but looking at the machine code I can see
what's happening:

Fpopen is returning a good directory descriptor, for example, 51345.
However, the comparison "fd<0" is evaluating to true, which causes the
routine to error out.

Looking at the machine code:

   0x00007ffff7038106 <+118>: callq 0x7ffff7037ee0
<SYSTEM_$$_FPOPEN$PCHAR$LONGINT$LONGINT$$LONGINT>
   0x00007ffff703810b <+123>: mov %eax,%ebx
   0x00007ffff703810d <+125>: cmp $0x0,%bx
   0x00007ffff7038112 <+130>: jl 0x7ffff703816b
<SYSTEM_$$_FPOPENDIR$PCHAR$$PDIR+219>

At +123 I can see that $eax/$ebx contains the correct descriptor, 51345.
However, when I print $bx in the debugger, it looks like a negative number,
as if it had overflowed a signed 2-byte integer, so the function errors
out. This causes the file descriptor to be leaked as well, since no
reference to is is retained in the pdir record.

Michael suggested we change the type of fd to a THandle or cInt to avoid the overflow that's happening due to (apparently?) the "integer" type being interpreted to mean a 16-bit signed value here.
Steps To Reproduce1. Have a linux system where you've set ulimit -n 64000 to increase the number of possible file handles.

2. open > 32*1024 file handles

3. call Fpopendir and see it fail as the file descriptor overflows
Additional InformationSee this thread:

http://lists.freepascal.org/pipermail/fpc-devel/2016-June/037189.html
TagsNo tags attached.
Fixed in Revision34039-34042
FPCOldBugId
FPCTarget
Attached Files

Activities

Seth Grover

2016-06-30 20:22

reporter   ~0093448

Looking at the definition of Dir/TDir:


{$ifdef oldreaddir}
           { Still old one. This is a userland struct}

   Dir = record
                dd_fd : integer;
                dd_loc : longint;
                dd_size : integer;
                dd_buf : pdirent;
                {The following are used in libc, but NOT in the linux kernel sources ??}
                dd_nextoff: cardinal;
                dd_max : integer; {size of buf. Irrelevant, as buf is of type dirent}
                dd_lock : pointer;
               end;
{$else}
        // new libc one. NOTE that off_t must be real, so 64-bit ifdef
   Dir = Record // packing doesn't matter. This is a userland struct.
                fd : cint;
                data : pchar;
                allocation: size_t;
                _size : size_t;
                offset : size_t;
                filepos : off_t;
                end;
{$endif}

I assume it's using the new libc structure, which defines fd as a cint. So It would probably be cleanest to define fd in the fixed Fpopendir as cint as well, since the type is definitely already going to be defined there.

Marco van de Voort

2016-06-30 20:46

manager   ~0093450

Done

Marco van de Voort

2016-06-30 20:55

manager   ~0093451

Last edited: 2016-06-30 20:56

View 2 revisions

Note oldreaddir was a quick workaround in 1.9.2. It was not meant to survive till 2.0.0, the whole construct is probably pre linux 2.0 when readdir was done by syscall (though the call was kept somewhat longer till 2.4.0 or so)

The whole of /usr/include in my linux distro doesn't grep on the fieldnames in TDir...

The *BSD ports directly went a getdirentries/getdents way, and didn't suffer from this mistake, it is longint since SVN revision 1.....

Seth Grover

2016-07-11 16:44

reporter   ~0093656

Looks good, here's the assembly after the patch:

...
   <+115>: callq 0x412520 <SYSTEM_$$_FPOPEN$PCHAR$LONGINT$LONGINT$$LONGINT>
   <+120>: mov %eax,%ebx
   <+122>: cmp $0x0,%ebx
   <+125>: jl 0x4127a6 <SYSTEM_$$_FPOPENDIR$PCHAR$$PDIR+214>
...

you can see the comparison now is against the full value of %ebx which causes the comparison to work properly.

Issue History

Date Modified Username Field Change
2016-06-30 20:14 Seth Grover New Issue
2016-06-30 20:22 Seth Grover Note Added: 0093448
2016-06-30 20:46 Marco van de Voort Fixed in Revision => 34039,34040
2016-06-30 20:46 Marco van de Voort Note Added: 0093450
2016-06-30 20:46 Marco van de Voort Status new => resolved
2016-06-30 20:46 Marco van de Voort Fixed in Version => 3.0.2
2016-06-30 20:46 Marco van de Voort Resolution open => fixed
2016-06-30 20:46 Marco van de Voort Assigned To => Marco van de Voort
2016-06-30 20:52 Marco van de Voort Fixed in Revision 34039,34040 => 34039-34042
2016-06-30 20:55 Marco van de Voort Note Added: 0093451
2016-06-30 20:56 Marco van de Voort Note Edited: 0093451 View Revisions
2016-07-11 16:44 Seth Grover Note Added: 0093656
2016-07-11 16:44 Seth Grover Status resolved => closed