Bug 172332 - [exp-run] Expanding stdio's internal file descriptors from short to int
Summary: [exp-run] Expanding stdio's internal file descriptors from short to int
Status: In Progress
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: John Baldwin
URL:
Keywords:
Depends on: 205029
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-04 19:50 UTC by John Baldwin
Modified: 2021-07-06 08:53 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description John Baldwin freebsd_committer freebsd_triage 2012-10-04 19:50:10 UTC
On Friday, September 28, 2012 6:47:39 pm John Baldwin wrote:
> Four years or so ago I cleaned up some of the stdio internals as fallout from 
> running into problems with stdio using a short instead of an int to hold file 
> descriptors.  Back then I got sidetracked with attempting to make FILE opaque 
> and ended up never getting around to bumping _file from a short to an int.  I 
> recently ran back into the SHRT_MAX limit at work again and came up with a 
> patch to fix this.
> 
> To preserve the ABI, it is necessary to leave the existing short _file in 
> place and add a new int _file to the end of the FILE structure.  Also, for old 
> applications, the old _file (_ofile in the patch) must still be valid.  The 
> approach I have taken is to bump the symbol version for routines that create 
> FILE objects with a non-fake _file (fopen, fdopen, and freopen).  The old 
> FBSD_1.0 variants still fail if an fd is greater than SHRT_MAX (and thus 
> cannot be safely stored in _ofile).  The new FBSD_1.3 variants assign to both 
> _file and _ofile if the fd is less than SHRT_MAX.  I also changed fileno()
> to no longer be an inline macro in <stdio.h> but to always be a function call 
> going forward.
> 
> If folks think this is ok, I'll hack up a modified version that hides _file
> from outside consumers (rename it to _nfile or some such) and send it for a
> ports-exp run before committing to make sure there aren't any 3rd party apps
> accessing _file directly.

I have a slightly modified version of my original patch that should hide _file
completely from any package builds.  No ports should be directly accessing the
internals of FILE.  They should be using things like fileno() instead.  Can
you do an exp-run with a patched world to see if there are any such abusers?
The patch is MI, so I think just doing one architecture should be sufficient.

http://www.FreeBSD.org/~jhb/patches/stdio_file_exp.patch
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2012-10-10 01:00:11 UTC
State Changed
From-To: open->analyzed
Comment 2 Mark Linimon freebsd_committer freebsd_triage 2012-10-10 01:00:11 UTC
Responsible Changed
From-To: freebsd-ports-bugs->linimon
Comment 3 Mark Linimon freebsd_committer freebsd_triage 2012-10-10 06:05:08 UTC
State Changed
From-To: analyzed->feedback

The first major failure is in, and it affects almost half the Ports 
Collection: 

http://pointyhat-west.isc.FreeBSD.org/errorlogs/amd64-errorlogs/a.10-exp.20121010024913.pointyhat-west/perl-5.14.2_2.log
Comment 4 Mark Linimon freebsd_committer freebsd_triage 2012-10-10 19:32:15 UTC
State Changed
From-To: feedback->suspended

Mark suspended awaiting updated patch.
Comment 5 Mark Linimon freebsd_committer freebsd_triage 2013-02-20 05:19:37 UTC
State Changed
From-To: suspended->suspended

I am no longer in a position to test this. 


Comment 6 Mark Linimon freebsd_committer freebsd_triage 2013-02-20 05:19:37 UTC
Responsible Changed
From-To: linimon->portmgr
Comment 7 Baptiste Daroussin freebsd_committer freebsd_triage 2014-06-15 22:48:38 UTC
Is this exp-run still needed?
Comment 8 John Baldwin freebsd_committer freebsd_triage 2015-10-21 17:17:11 UTC
Prompted by a discussion yesterday I cleaned up this patchset and forward ported it to HEAD.  I do still need an exp-run of an updated patch.  The summary so far is this:

1) perl abuses _file and needs to be changed to use fdclose() instead.  oshogbo@ has a patch for Perl itself, but not patches for the various perl ports.  We would need to get all the Perl ports patched.  I believe Mariusz has been trying to get the Perl patch upstreamed.  I'm not sure if the patch needs to be tweaked to only use fdclose() on sufficiently new __FreeBSD_version values (right now it just always uses fdclose()).

2) I have a modified version of the patch that should allow Perl to build but hide _file from other ports.  I had misremembered and thought this had been done and that I had more stuff to fix, but apparently this was not run yet.  This is the next exp-run I need done.

The current patchset (including the Perl hack) is on github at github/bsdjhb/freebsd in the stdio_file branch.  I can generete a patch if you want or you can just extract it from git directly.  The patch is relative to HEAD.
Comment 9 Antoine Brodin freebsd_committer freebsd_triage 2015-10-22 11:45:49 UTC
Can you extract the patch?
Comment 10 John Baldwin freebsd_committer freebsd_triage 2015-10-22 20:21:14 UTC
(In reply to Antoine Brodin from comment #9)
You can fetch it here:

https://github.com/freebsd/freebsd/compare/master...bsdjhb:stdio_file.patch

(Will need to use patch -p1 to apply it due to git's insistence on the a/b prefixes)
Comment 11 Antoine Brodin freebsd_committer freebsd_triage 2015-10-25 13:21:43 UTC
Exp-run results on amd64:

http://package18.nyi.freebsd.org/jail.html?mastername=headamd64PR172332-default

6 new failures:

+ {"origin"=>"databases/tarantool", "pkgname"=>"tarantool-1.6.6", "phase"=>"build", "errortype"=>"new_compiler_error"}
+ {"origin"=>"games/xpipeman", "pkgname"=>"xpipeman-1.0_4", "phase"=>"build", "errortype"=>"clang"}
+ {"origin"=>"games/xrobots", "pkgname"=>"xrobots-1.0_5", "phase"=>"build", "errortype"=>"clang"}
+ {"origin"=>"lang/gcl", "pkgname"=>"gcl-2.6.12_1", "phase"=>"build", "errortype"=>"new_compiler_error"}
+ {"origin"=>"multimedia/kodi", "pkgname"=>"kodi-15.1_1", "phase"=>"build", "errortype"=>"bad_C++_code"}
+ {"origin"=>"multimedia/plexhometheater", "pkgname"=>"plexhometheater-1.4.1_1", "phase"=>"build", "errortype"=>"clang-bug"}

Failure logs:

http://package18.nyi.freebsd.org/data/headamd64PR172332-default/2015-10-25_11h40m14s/logs/errors/tarantool-1.6.6.log
http://package18.nyi.freebsd.org/data/headamd64PR172332-default/2015-10-25_11h40m14s/logs/errors/xpipeman-1.0_4.log
http://package18.nyi.freebsd.org/data/headamd64PR172332-default/2015-10-25_11h40m14s/logs/errors/xrobots-1.0_5.log
http://package18.nyi.freebsd.org/data/headamd64PR172332-default/2015-10-25_11h40m14s/logs/errors/gcl-2.6.12_1.log
http://package18.nyi.freebsd.org/data/headamd64PR172332-default/2015-10-25_11h40m14s/logs/errors/kodi-15.1_1.log
http://package18.nyi.freebsd.org/data/headamd64PR172332-default/2015-10-25_11h40m14s/logs/errors/plexhometheater-1.4.1_1.log
Comment 12 Antoine Brodin freebsd_committer freebsd_triage 2015-10-26 19:46:32 UTC
Exp-run results on i386:

http://pb2.nyi.freebsd.org/jail.html?mastername=headi386PR172332-default

The new failures are the same as on amd64
Comment 13 Steve Read 2020-04-27 08:22:59 UTC
Hello,

I'd like to bump the visibility of this bug because it is causing us problems.  We have a daemon in our system that has *lots* of sockets open, and by "lots", I mean potentially tens of thousands, and when we poke it to reload its configuration, it fails because the configuration file gets an FD above SHRT_MAX, so fdopen fails with EMFILES.

Simply changing the field and recompiling everything isn't feasible (or even possible) for us because we have some precompiled third-party binaries (not actually used *by* the proxy, but in the system) that use stdio files.  (Or at the very least, they might use them.)

-- Steve Read
Stormshield.