bugmake - Bugs: bug #15534, directory_contents_hash_cmp broken...

 
 

bug #15534: directory_contents_hash_cmp broken by 64 bit inode numbers

Submitted by:  Martin Dorey <mdorey>
Submitted on:  Thu 26 Jan 2006 08:12:01 AM UTC  
 
Severity: 3 - NormalItem Group: Bug
Status: FixedPrivacy: Public
Assigned to: Paul D. Smith <psmith>Open/Closed: Closed
Component Version: 4.0Operating System: Any
Fixed Release: 3.81Triage Status: None

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Sun 05 Feb 2006 04:17:34 PM UTC, comment #3:

Fixed for the next version of GNU make.

Paul D. Smith <psmith>
Project AdministratorIn charge of this item.
Thu 26 Jan 2006 02:50:44 PM UTC, comment #2:

(The inode number is already stored in an ino_t.) I guess we should be similarly careful about comparing dev_t and in comparing the 3 ino_t values used on VMS.

Martin Dorey <mdorey>
Thu 26 Jan 2006 01:11:03 PM UTC, comment #1:

The cmp function needs to behave properly for ordering, even if it's not sorted now. Breaking the contract is a horrible bug waiting to happen. If we need to store inodes we should be using ino_t, with the proper autoconf magic to make that type available on all systems.

But, as you say I think in this particular case we just have to be more careful about writing the comparison.

Paul D. Smith <psmith>
Project AdministratorIn charge of this item.
Thu 26 Jan 2006 08:12:01 AM UTC, original submission:

The non-Win32, non-VMS code for directory_contents_hash_cmp bogusly returns equality when two 64 bit inode numbers differ only in the top 32 bits, when the device number is the same and when the machine's natural int width is 32 bits. This isn't the hash function - this is the comparison function. So this can cause two distinct directories to be treated as if they were the same.

You may ask how on earth did I run into this. Well, I think that unlike Cygwin 1.5.18, Cygwin 1.5.19's stat(2) uses the inode numbers returned by Samba, because of this change (and subsequent clarifications):

http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/cygwin/fhandler_disk_file.cc.diff?r1=1.147&r2=1.148&cvsroot=src&f=h

Because of the issue documented here:

https://bugzilla.samba.org/show_bug.cgi?id=3287

Some and probably many versions of Samba return the remote inode number in the top 32 bits of the result of eg the Trans2 QUERY_FILE_INFO, Query File Internal Info request, using the bottom 32 bits for the remote device number. Cygwin faithfully copies this into the 64 bit inode number.

make (3.81beta4) is then subtracting one 64 bit inode number from another and assigning the result to what, on my (reasonable) system, is a 32 bit variable:

static int
directory_contents_hash_cmp (const void xv, const void yv)
...
int result;
...
result = x->ino - y->ino;
if (result)
return result;

I know that Windows support is something of a bone of contention but this is, I think, of potentially wider importance, although you'd probably have to be blisteringly unlucky to hit the problem.

64 bit inode numbers aren't such a rarity these days. NFSv3 allows them. They can be especially useful for file systems which support snapshotted versions of files. Whereas 32 bit int still seems to be the norm.

I know it's gruesome but perhaps we could do something like:

/*
* Subtracting ino_t values and assigning the result to
* an int can be lossy.
*/
result = x->ino > y->ino ? 1 : x->ino < y->ino ? -1 : 0;

I see that hash.c doesn't (currently) sort its hash buckets, so we could perhaps get away with:

result = x->ino != y->ino;

If we were going to take that approach, I guess we'd want to change the other subtractions in the same function in the same way. Or leave a comment by the odd man out.

Martin Dorey <mdorey>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

Do you think this task is very important?
If so, you can click here to add your encouragement to it.
This task has 0 encouragements so far.

Only logged-in users can vote.

 

Please enter the title of George Orwell's famous dystopian book (it's a date):

 

 

Follow 5 latest changes.

Date Changed By Updated Field Previous Value => Replaced By
Sat 01 Apr 2006 07:18:52 AM UTCpsmithFixed Release4.0=>3.81
Sun 05 Feb 2006 04:17:34 PM UTCpsmithStatusNone=>Fixed
  Assigned toNone=>psmith
  Open/ClosedOpen=>Closed
  Fixed ReleaseNone=>4.0

Back to the top


Powered by Savane 3.1-cleanup1