perl rounds inode in PP stat #15002
Comments
From @bulk88Created by @bulk88While I was thinking about implementing real inode info in Win32's PP stat()[1], I discovered that Perl rounds the inode integer. http://perl5.git.perl.org/perl.git/commitdiff/8d8cba88681398d40004e97bcb93f7f8b88ae05e associated ticket https://rt.perl.org/Public/Bug/Display.html?id=84590 An inode has 2 uses AFAIK. -undelete a file An inode is effectively an opaque pointer (integer) into a FS. If the OS's st_ino is 64b (Win32 inode is always 64b per FS driver API) on 32b IV perl, it has the potential of being rounded if its > 2^53 and therefore is garbage/uninitialized. In a DB on FS scheme, bad things could happen if 2 files that aren't links in reality, "==" in perl as to being the same file. An inode can be implemented in a couple ways by any FS. -a 0 based offset into an array of something (offsets or structs) on the disk [1] https://rt.perl.org/Public/Bug/Display.html?id=65052 Everyone agrees storing 64 bit C pointers in 64 bit doubles is forbidden, so why is perl storing inodes in NVs/doubles? Can something be done about this? Fatally error if its over 2^32 or over 2^53? Store the 64 bit integer as a SVPV in printable ASCII and let the user in PP figure out what to do with it (Math::Int64 it)? Perl Info |
From @bulk88On Tue Oct 20 18:46:47 2015, bulk88 wrote:
Actually, MS bumped the inode size to 128 bits in Server 2012 OS for their nextgen enterpriseish FS. So for discussion, assume 128 bit inodes are real and have to be supported (personally Win32 Perl can stick with legacy 64 bit inodes for some years). https://msdn.microsoft.com/en-us/library/windows/desktop/hh802691%28v=vs.85%29.aspx See also https://msdn.microsoft.com/en-us/library/windows/desktop/aa363788%28v=vs.85%29.aspx -- |
From @tonycozOn Tue Oct 20 18:46:47 2015, bulk88 wrote:
I can see us producing some sort of error if the inode number changes value when stored as a NV, perhaps with an option to disable that error, since stat() isn't only used to fetch a file's inode number. As to how to behave when the inode number doesn't fit, we can look at existing implementations, from the Solaris stat() man page: The stat(), fstat(), and lstat() functions may fail if: EOVERFLOW One of the members is too large to store in the Similarly on FreeBSD: [EOVERFLOW] The file size in bytes cannot be represented correctly If we follow the lead of Solaris/FreeBSD, perl's stat() would simply fail with EOVERFLOW (if available) if the inode number doesn't fit. As to optionally disabling the error, we could use a variable like Sort of related: systems with large files can run into this issue for st_size: $ ./perl -le 'print +(stat "/home/tony/somefile.txt")[7]' (that's a sparse file on ZFS) Tony |
|
The RT System itself - Status changed from 'new' to 'open' |
From @ap* bulk88 <perlbug-followup@perl.org> [2015-10-21 03:50]:
Like a zip code. That is also nominally a number but is meaningless
If you mean converting the inode number 9876543210123456 to the string * Tony Cook via RT <perlbug-followup@perl.org> [2015-10-27 04:50]:
That would force all code not written for a controlled environment to If we’re going to go this route it would be nice to only force code that Regards, |
From @bulk88On Tue Oct 27 00:54:49 2015, aristotle wrote:
Although I am hesitant about returning PVs and "eq" instead of "==" for perf reasons, that might be the only sane way to support 128 bit inodes. If an inode number is really a string, maybe it should be raw 4/8/16 bytes of binary, not printable ascii. Integer math on it is illegal and makes no sense. Lets take a 128bit number, 0x3EB8E79140631092131F93905E8CB80E, 32 bytes to print in HEX ASCII. 16 bytes in binary. 38 bytes in decimal ASCII. Since inode comparison is rare, I was thinking either warn or die if it is > 2^53 (overflows NV). Even if it were raw binary bytes SVPVs are fatter than SVNVs and SVIVs. Someone with ZFS or whatever might be returning >2^53 inodes all the time that makes PP stat unusable with continuous warnings and dies. Another choice is, if it overflows, the inode is simply 0, an invalid (is inode 0 legal per posix?) constant that any human should know something is wrong.
Attaching magic (upgrade to SVMG, allocate a MG body, fill it in, etc) is expensive unless there is an immortable global SV* that is "fetch forbidden" and set up once per perl process. -- |
From @AbigailOn Tue, Oct 27, 2015 at 09:51:44AM -0700, bulk88 via RT wrote:
From my hazy memory, 0 isn't illegal, but no file system [1] actually Abigail |
From @ap* bulk88 via RT <perlbug-followup@perl.org> [2015-10-27 17:55]:
Yes. It is rare. And since eq does its level best to short-circuit, the I proposed conversion to decimal for two reasons: 1. You can leave non-overflowing inode numbers as IVs, so the cost is at 2. Usability basically. Say you want to print the inode number in some Converting to hex would satisfy the usability consideration but unless Basically converting overflowing values to decimal strings seems to be
I’m not sure I follow here; first you seem to state a preference for
I guess, but… that has the same problem as dying: it makes stat useless -- |
From @bulk88On Tue Oct 20 18:46:47 2015, bulk88 wrote:
Since the code freezes begun/coming, I wrote a patch with the most conservative solution (instead of "eq" instead of "==" solutions like PVs of hex strings or raw bytes or PVs of decimal integers) so this ticket can be closed. This patch that I attached is part of a near future win32_stat refactor patch that I split up. -- |
From @bulk880001-perl-126414-add-warning-if-perl-rounds-inode-in-PP-s.patchFrom e9dda1b0e3c7c0721d983604d685a6362a39f3f6 Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Fri, 5 Feb 2016 02:43:36 -0500
Subject: [PATCH] [perl #126414] add warning if perl rounds inode in PP stat
Do the most conservative approach to fixing #126414, warn if rounding
occurred. Since on some CCs/platforms integer to FP casting is a
internally a function call, only do it once. See also #84590 and
commit 8d8cba8868 .
---
pod/perldiag.pod | 7 +++++++
pp_sys.c | 13 ++++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index ae061f9..d22dc66 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -2733,6 +2733,13 @@ transparently promotes all numbers to a floating point representation
internally--subject to loss of precision errors in subsequent
operations.
+=item Integer overflow in stat inode
+
+(S overflow) Your system's inode type is larger than perl can represent
+on your system's archtecture, and this particular inode value returned by
+your system's stat was stored by perl as a floating pointer number with a loss
+of precision.
+
=item Integer overflow in srand
(S overflow) The number you have passed to srand is too big to fit
diff --git a/pp_sys.c b/pp_sys.c
index 15b4d8b..a7d99d1 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -2965,11 +2965,22 @@ PP(pp_stat)
RETURN;
}
if (max) {
+#if ST_INO_SIZE > IVSIZE
+ NV ino;
+#endif
EXTEND(SP, max);
EXTEND_MORTAL(max);
mPUSHi(PL_statcache.st_dev);
#if ST_INO_SIZE > IVSIZE
- mPUSHn(PL_statcache.st_ino);
+ ino = (NV)PL_statcache.st_ino;
+ if(
+# if ST_INO_SIGN <= 0
+ ino < -NV_OVERFLOWS_INTEGERS_AT ||
+# endif
+ ino > NV_OVERFLOWS_INTEGERS_AT)
+ Perl_ck_warner_d(aTHX_ packWARN(WARN_OVERFLOW),
+ "Integer overflow in stat inode");
+ mPUSHn(ino);
#else
# if ST_INO_SIGN <= 0
mPUSHi(PL_statcache.st_ino);
--
1.9.5.msysgit.1
|
From @bulk88On Thu Feb 04 23:49:50 2016, bulk88 wrote:
Actually, I'm not sure if this patch should be applied before 5.24. In a separate patch, I added inode support to Win32 Perl, and it seems that NTFS inode numbers are a 64 bit bitfield/struct, not linear 64 bit counter, and therefore always > 2^53 and every PP stat() is generating an overflow warning on my 32 bit IV perl. Not sure how to proceed now. Example output from a modified perl Integer overflow in stat inode hi 00320000 lo 000451e6 fp 14073748835815910.0000 -- |
From @ap* bulk88 via RT <perlbug-followup@perl.org> [2016-02-10 02:30]:
Which implies that the inode field has always been broken. Not bad! I must wonder how come nobody ran into this before, then. An impressive Presumably by way of the rounding being deterministic. Possibly due to
Test that hypothesis. Run a recursive walk over your FSs and record the Regards, |
From @bulk88On Thu Feb 11 15:55:50 2016, aristotle wrote:
The windows libc ALWAYS sets st_ino to 0. Blead win32 perl's st_ino is always 0 ( there is an old feature request in https://rt.perl.org/Ticket/Display.html?id=65052 asking to fix that I attempted to fix). I created an unpublished/yet to publish patch that gets the inode ("FileIndex" or "IndexNumber" https://msdn.microsoft.com/en-us/library/windows/hardware/ff540318%28v=vs.85%29.aspx ) from the MS Win32 API, since the MS libc API doesn't supply it and presents it on a Perl C level to upper Perl C layers. That is how I got inodes visible on a PP level in win32 perl and was able to test this overflow/rounding patch.
The chances of a collision seem low, but not theoretically impossible (only comparing all 64 bits would be collision free). Since Im not a CS PHD, I wont try to estimate the chance. Any porters can analyze my raw data that i posted and guess the chance. The highest 16 bit short seems to be a bucket or checksum, while the low 32 bits seem to be an offset into an array. That raw data is probably from iterating over my perl git source code root dir and some of the offsets seem very close.
Iterate over the entire FS, make a hash with the key being the FP and value 1 or ++. At the end of interating over the entire FS (or as much before 32 bit process mem limit hits), go over the hash, if a key has a value >1, either I have hard links on Win32 or rounding collision. Is this the correct algorithm? -- |
From zefram@fysh.orgFixed in commit 2e8ea15. stat() now -zefram |
From @shlomifOn Sat, 11 Nov 2017 07:50:52 +0000
Thanks, Zefram! -- Shlomi Fish http://www.shlomifish.org/ XSLT is the number one cause of programmers’ suicides since Visual Basic 1.0. Please reply to list if it's a mailing list post - http://shlom.in/reply . |
From @dur-randirOn Fri, 10 Nov 2017 23:51:30 -0800, zefram@fysh.org wrote:
This commit introduced a new build warning on my machine (64 bit Mac OS X): pp_sys.c:3014:32: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare] |
From @jkeenanOn Sat, 11 Nov 2017 15:27:16 GMT, randir wrote:
Can you attach the output of 'perl -V' from this build? Thank you very much. -- |
From @dur-randirOn Sat, 11 Nov 2017 08:22:22 -0800, jkeenan wrote:
Summary of my perl5 (revision 5 version 27 subversion 6) configuration: Characteristics of this binary (from libperl): |
From zefram@fysh.orgSergey Aleynikov via RT wrote:
Thanks. That should be fixed by commit -zefram |
From @dur-randirOn Sat, 11 Nov 2017 15:27:55 -0800, zefram@fysh.org wrote:
Yes, this fixed it, thanks. |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release yesterday of Perl 5.28.0, this and 185 other issues have been Perl 5.28.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
|
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#126414 (status was 'resolved')
Searchable as RT126414$
The text was updated successfully, but these errors were encountered: