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

 
 

bug #15534: directory_contents_hash_cmp broken by 64 bit inode numbers

Submitter:  Martin Dorey <mdorey>
Submitted:  Thu 26 Jan 2006 08:12:01 AM UTC
   
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  psmith Open/Closed:  Closed
Component Version:  4.0 Operating System:  Any
Fixed Release:  3.81 Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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

Fixed for the next version of GNU make.

Paul D. Smith <psmith>
Group administrator
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>
Group administrator
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 Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

CC list is empty

 

There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

Only logged-in users can vote.

 

Follow 5 latest changes.

Date Changed by Updated Field Previous Value => Replaced by
2006-04-01 psmith Fixed Release4.0 3.81
2006-02-05 psmith StatusNone Fixed
    Assigned toNone psmith
    Open/ClosedOpen Closed
    Fixed ReleaseNone 4.0

Back to the top

Powered by Savane 3.13-4448.
Corresponding source code