Package: hfsprogs Version: 540.1.linux3-4 Severity: normal Tags: patch upstream X-Debbugs-Cc: [email protected]
Dear Maintainer, Apple (the upstream) never finished the implementation of the directory hardlink support in fsck_hfs. Since, once again, my TimeMachine backup rendered unrepairable due to directory hardlink errors, I implemented what was required to fix the errors I encountered. My trust in Apple's feedback system is very low so I am sending the patch to you in the hopes that you can forward it to Apple. The patch I added should work, with offsets, on both, Debian's sources as well as Apple's sources. My changes: - Any hardlink inode with a parent other than the metadata directory is changed to have the correct parent. I have seen these. I can understand how this happens when a normal directory is hardlined for the first time. The journal SHOULD prevent this from being a problem but obviously this is not the case. - Directory hardlink inodes named "temp..." get deleted. These directories were deleted while open when the volume was removed without unmounting. Since these directories have already been deleted, deleting them in fsck is the right thing to do. (Documented like that for files in the HFS+ spec.) - Directory hardlink inodes with names other than "temp..." or "dir_..." are moved to "lost+found". It would be better to rename them to "dir_..." in a first pass, there might still be valid links to them. These links are deleted by a different repair step and I did not see how to remove those other repair steps. Would probably lead to a bigger rewrite of the hardlink check code so I decided to just move these to l+f. I have never seen this anyways. After all, none of the errors should happen in a journaled filesystem (directory hardlinks require the Journal to be active). My guess is that using HFS+ over AFP (as TimeMachine does) does not correctly persist the journal before persisting changes. Thus loosing the connection during directory hardlink operation can result in partially executed atomic operation. I will attach the patch and test cases for each of the three fixed errors. These test cases have been constructed in a hex editor, they might not be what a real error looks like. The first two repairs successfully repaired my broken backup file so I guess they work (sometimes). Bye, Daniel -- System Information: Debian Release: 11.0 APT prefers stable-security APT policy: (500, 'stable-security'), (500, 'stable') Architecture: amd64 (x86_64) Kernel: Linux 5.10.0-8-amd64 (SMP w/2 CPU threads) Locale: LANG=en_US.utf8, LC_CTYPE=en_US.utf8 (charmap=UTF-8), LANGUAGE=en_US:en Shell: /bin/sh linked to /bin/bash Init: systemd (via /run/systemd/system) LSM: AppArmor: enabled Versions of packages hfsprogs depends on: ii libc6 2.31-13 ii libssl1.1 1.1.1k-1+deb11u1 hfsprogs recommends no packages. hfsprogs suggests no packages.
>From 995c492cc8f1c55330c756ff22e634f831077e3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20H=C3=B6pfl?= <[email protected]> Date: Wed, 15 Sep 2021 10:55:43 +0200 Subject: [PATCH] Implemented directory hardlink support --- fsck_hfs.tproj/dfalib/SRepair.c | 230 +++++++++++++++++++++++++++- fsck_hfs.tproj/dfalib/dirhardlink.c | 98 ++++++++++-- fsck_hfs.tproj/fsck_hfs_msgnums.h | 6 +- 3 files changed, 313 insertions(+), 21 deletions(-) diff --git a/fsck_hfs.tproj/dfalib/SRepair.c b/fsck_hfs.tproj/dfalib/SRepair.c index 405f1a0..5cb7905 100644 --- a/fsck_hfs.tproj/dfalib/SRepair.c +++ b/fsck_hfs.tproj/dfalib/SRepair.c @@ -33,6 +33,7 @@ #include "Scavenger.h" #include <unistd.h> +#include <inttypes.h> #include <sys/stat.h> #include <stdlib.h> #include "../cache.h" @@ -119,7 +120,6 @@ static int BuildThreadRec( CatalogKey * theKeyPtr, CatalogRecord * theRecPtr, static int BuildFileRec(UInt16 fileType, UInt16 fileMode, UInt32 fileID, Boolean isHFSPlus, CatalogRecord *catRecord); static void BuildAttributeKey(u_int32_t fileID, u_int32_t startBlock, unsigned char *attrName, u_int16_t attrNameLen, HFSPlusAttrKey *key); - OSErr RepairVolume( SGlobPtr GPtr ) { OSErr err; @@ -982,7 +982,7 @@ OSErr FixFileHardLinkFlag(SGlobPtr GPtr, RepairOrderPtr p) Routine: FixPrivDirBadPerms - fix the permissions of the directory hard-link private dir Input: GPtr -- pointer to scavenger global data - p -- poitner to a minor repair order + p -- pointer to a minor repair order Output: function result: 0 -- no error @@ -1025,6 +1025,223 @@ done: return retval; } +/* +Routine: FixBadParentInode - move inode file/folder to metadata directory. + +Input: GPtr -- pointer to scavenger global data + p -- pointer to a minor repair order + +Output: function result: + 0 -- no error + n -- error +*/ + +static OSErr FixBadParentInode(SGlobPtr GPtr, RepairOrderPtr p) +{ + OSErr retval = noErr; + UInt32 hint; + + CatalogName inodeName; + + CatalogKey incorrectKey; + CatalogKey correctKey; + + CatalogRecord currentRecord; + uint16_t currentRecordSize; + + CatalogKey parentFolderKey; + CatalogRecord parentFolderRecord; + uint16_t parentFolderRecordSize; + + /* This error only happens on HFS+ */ + if ( !VolumeObjectIsHFSPlus() ) { + retval = IntError( GPtr, R_IntErr ); + goto done; + } + + inodeName.ustr = *((HFSUniStr255 *)p->name); + + // Read record data using current (incorrect) key + BuildCatalogKey(p->incorrect, &inodeName, true, &incorrectKey); + retval = SearchBTreeRecord(GPtr->calculatedCatalogFCB, &incorrectKey, kNoHint, NULL, ¤tRecord, ¤tRecordSize, NULL); + if ( retval != noErr ) { + if ( retval == btNotFound ) { + retval = 0; + } + goto done; + } + + // We do not want to ever loose the record, so we insert/replace the new one first. + BuildCatalogKey(p->correct, &inodeName, true, &correctKey); + retval = ReplaceBTreeRecord( GPtr->calculatedCatalogFCB, &correctKey, kNoHint, ¤tRecord, currentRecordSize, &hint); + if ( retval == btNotFound ) { + // Could not replace because it was not there before, so insert it. + retval = InsertBTreeRecord( GPtr->calculatedCatalogFCB, &correctKey, ¤tRecord, currentRecordSize, &hint); + } + if ( retval != noErr ) { + goto done; + } + + // Now we delete the incorrect one. + retval = DeleteBTreeRecord( GPtr->calculatedCatalogFCB, &incorrectKey ); + if ( retval != noErr ) { + goto done; + } + + /* NOTE: At this point, the valence of one or both of the folders is + * most likely incorrect. To know which one, one has to count + * the items in the folders. This is one of the repair steps + * anyways so it is okay to just ignore this here and let the + * next repair pass handle it. + */ + GPtr->minorRepairFalseSuccess = true; + +done: + if ( retval != noErr ) { + if ( fsckGetVerbosity(GPtr->context) >= kDebugLog ) { + plog( "\t%s - repair failed for type 0x%02X %d \n", __FUNCTION__, p->type, p->type ); + } + } + + return retval; +} + +/* +Routine: FixDirInodeBadName - fix the name of a hard linked folder. + +Input: GPtr -- pointer to scavenger global data + p -- pointer to a minor repair order + +Output: function result: + 0 -- no error + n -- error +*/ + +static OSErr FixDirInodeBadName(SGlobPtr GPtr, RepairOrderPtr p) +{ + OSErr retval = 0; + + SFCB *fcbPtr; + UInt32 hint; + + size_t *incorrect_len; + char *incorrect; + char *correct; + + CatalogKey currentKey; + CatalogRecord currentRecord; + UInt16 currentRecordSize; + + UInt32 threadID; + CatalogKey threadKey; + CatalogRecord threadRecord; + UInt16 threadRecordSize; + + CatalogKey parentKey; + CatalogRecord parentRecord; + UInt16 parentRecordSize; + + UInt32 oldValence; + UInt32 oldFolderCount; + + + /* This error only ever happens on HFS+ */ + if ( !VolumeObjectIsHFSPlus() ) { + goto done; + } + + incorrect_len = (size_t *)p->name; + incorrect = (char *) p->name + sizeof(size_t); + correct = (char *) p->name + sizeof(size_t) + *incorrect_len + 1; + + fcbPtr = GPtr->calculatedCatalogFCB; + + retval = GetCatalogRecordByID( GPtr, (UInt32)p->parid, true, ¤tKey, ¤tRecord, ¤tRecordSize ); + if ( retval != noErr ) { + if ( retval == btNotFound ) { + /* If the record was not found this means that the record was + * deleted by an other repair order, return success since this + * means the name is no longer used. Make it a false success + * to be sure. + */ + GPtr->minorRepairFalseSuccess = true; + retval = 0; + } + goto done; + } + + if ( currentRecord.recordType == kHFSPlusFolderRecord ) { + threadID = currentRecord.hfsPlusFolder.folderID; + } else if ( currentRecord.recordType == kHFSPlusFileRecord ) { + threadID = currentRecord.hfsPlusFile.fileID; + } else { + retval = IntError( GPtr, R_IntErr ); + goto done; + } + + // Is it a deleted link that was open when unmount happened? + if (strncmp(incorrect, HFS_DELETE_PREFIX, strlen(HFS_DELETE_PREFIX)) == 0) { + retval = DeleteBTreeRecord( fcbPtr, ¤tKey ); + if ( retval != noErr ) { + if ( fsckGetVerbosity(GPtr->context) >= kDebugLog ) { + plog( "\t%s - failed to delete inode record %" PRIu32 "\n", __FUNCTION__, p->parid ); + } + goto done; + } + + BuildCatalogKey( threadID, NULL, true, &threadKey ); + retval = SearchBTreeRecord( fcbPtr, &threadKey, kNoHint, NULL, &threadRecord, &threadRecordSize, NULL ); + if ( retval == noErr ) { + retval = DeleteBTreeRecord( fcbPtr, &threadKey ); + if ( retval != noErr ) { + if ( fsckGetVerbosity(GPtr->context) >= kDebugLog ) { + plog( "\t%s - failed to delete thread record for folder ID %" PRIu32 "\n", __FUNCTION__, threadID ); + } + goto done; + } + } else { + if ( fsckGetVerbosity(GPtr->context) >= kDebugLog ) { + plog( "\t%s - no thread record to delete for folder ID %" PRIu32 "\n", __FUNCTION__, threadID ); + } + } + + retval = GetCatalogRecordByID( GPtr, currentKey.hfsPlus.parentID, true, &parentKey, &parentRecord, &parentRecordSize ); + if ( retval != noErr ) { + goto done; + } + + oldValence = parentRecord.hfsPlusFolder.valence; + oldFolderCount = parentRecord.hfsPlusFolder.folderCount; + + if ( parentRecord.hfsPlusFolder.valence > 0 ) { + parentRecord.hfsPlusFolder.valence--; + } + if ( currentRecord.recordType == kHFSPlusFolderRecord && (parentRecord.hfsPlusFolder.flags & kHFSHasFolderCountMask) && parentRecord.hfsPlusFolder.folderCount > 0 ) { + parentRecord.hfsPlusFolder.folderCount--; + } + + if ( (parentRecord.hfsPlusFolder.valence != oldValence) || (parentRecord.hfsPlusFolder.folderCount != oldFolderCount) ) { + retval = ReplaceBTreeRecord( GPtr->calculatedCatalogFCB, &parentKey, kNoHint, &parentRecord, parentRecordSize, &hint ); + if ( retval != noErr ) { + goto done; + } + } else { + } + } else { // not a pending delete, actually incorrect name, moving to lost+found. + retval = FixOrphanInode(GPtr, p); + goto done; + } + +done: + if ( retval != noErr ) { + if ( fsckGetVerbosity(GPtr->context) >= kDebugLog ) { + plog( "\t%s - repair failed for type 0x%02X %d \n", __FUNCTION__, p->type, p->type ); + } + } + return retval; +} + + /*------------------------------------------------------------------------------ Routine: FixOrphanLink @@ -1510,6 +1727,15 @@ static OSErr DoMinorOrders( SGlobPtr GPtr ) // the globals err = FixPrivDirBadPerms(GPtr, p); break; + case E_DirInodeBadParent: + case E_FileInodeBadParent: + err = FixBadParentInode(GPtr, p); + break; + + case E_DirInodeBadName: + case E_FileInodeBadName: + err = FixDirInodeBadName(GPtr, p); + case E_InvalidLinkChainFirst: err = FixBadLinkChainFirst(GPtr, p); break; diff --git a/fsck_hfs.tproj/dfalib/dirhardlink.c b/fsck_hfs.tproj/dfalib/dirhardlink.c index ed1b0ed..bd6c289 100644 --- a/fsck_hfs.tproj/dfalib/dirhardlink.c +++ b/fsck_hfs.tproj/dfalib/dirhardlink.c @@ -208,32 +208,88 @@ int record_inode_badflags(SGlobPtr gptr, uint32_t inode_id, Boolean isdir, } /* Record minor repair order for invalid parent of directory/file inode */ -/* XXX -- not repaired yet (file or directory) */ static int record_inode_badparent(SGlobPtr gptr, uint32_t inode_id, Boolean isdir, - uint32_t incorrect, uint32_t correct) + uint32_t incorrect, uint32_t correct, HFSUniStr255 *name) { + RepairOrderPtr p; char str1[12]; char str2[12]; - fsckPrint(gptr->context, isdir? E_DirInodeBadParent : E_FileInodeBadParent, inode_id); - snprintf(str1, sizeof(str1), "%u", correct); - snprintf(str2, sizeof(str2), "%u", incorrect); - fsckPrint(gptr->context, E_BadValue, str1, str2); + p = AllocMinorRepairOrder(gptr, sizeof(HFSUniStr255)); + if (p == NULL) { + return ENOMEM; + } - gptr->CatStat |= S_LinkErrNoRepair; + p->type = isdir ? E_DirInodeBadParent : E_FileInodeBadParent; + p->correct = correct; + p->incorrect = incorrect; + p->parid = inode_id; + memcpy(p->name, name, sizeof(HFSUniStr255)); + + gptr->CatStat |= S_LinkErrRepair; + + if ( IsDuplicateRepairOrder(gptr, p) == 1 ) { + DeleteRepairOrder(gptr, p); + } else { + fsckPrint(gptr->context, p->type, inode_id); + snprintf(str1, sizeof(str1), "%u", correct); + snprintf(str2, sizeof(str2), "%u", incorrect); + fsckPrint(gptr->context, E_BadValue, str1, str2); + } return 0; } /* Record minor repair order for invalid name of directory inode */ -/* XXX - not repaired yet (file or directory) */ static int record_inode_badname(SGlobPtr gptr, uint32_t inode_id, - char *incorrect, char *correct) + Boolean isdir, char *incorrect, char *correct) { - fsckPrint(gptr->context, E_DirInodeBadName, inode_id); - fsckPrint(gptr->context, E_BadValue, correct, incorrect); + RepairOrderPtr p; + size_t *sizeptr; + char *charptr; + + p = AllocMinorRepairOrder(gptr, sizeof(size_t) + strlen(incorrect) + 1 + strlen(correct) + 1); + if (p == NULL) { + return ENOMEM; + } + + p->type = isdir ? E_DirInodeBadName : E_FileInodeBadName; + p->parid = inode_id; + + // Store names: len(incorrect), incorrect, \0, correct, \0 + sizeptr = (size_t *)p->name; + *sizeptr = strlen(incorrect); + charptr = p->name + sizeof(size_t); + charptr = stpcpy(charptr, incorrect) + 1; + strcpy(charptr, correct); - gptr->CatStat |= S_LinkErrNoRepair; + gptr->CatStat |= S_LinkErrRepair; + + if ( IsDuplicateRepairOrder(gptr, p) == 1 ) { + DeleteRepairOrder(gptr, p); + } else { + fsckPrint(gptr->context, p->type, inode_id); + fsckPrint(gptr->context, E_BadValue, correct, incorrect); + } + + return 0; +} + +/* Record minor repair order for orphaned inode */ +static int record_orphan_inode(SGlobPtr gptr, uint32_t inode_id, Boolean isdir) +{ + RepairOrderPtr p; + + fsckPrint(gptr->context, isdir ? E_OrphanDirInode : E_OrphanFileInode, inode_id); + + p = AllocMinorRepairOrder(gptr, 0); + if (p == NULL) + return ENOMEM; + + p->type = isdir ? E_OrphanDirInode : E_OrphanFileInode; + p->parid = inode_id; + + gptr->CatStat |= S_LinkErrRepair; return 0; } @@ -621,7 +677,7 @@ int inode_check(SGlobPtr gptr, PrimeBuckets *bucket, /* inode should only reside in its corresponding private directory */ if ((parentid != 0) && (key->hfsPlus.parentID != parentid)) { - (void) record_inode_badparent(gptr, inode_id, isdir, key->hfsPlus.parentID, parentid); + (void) record_inode_badparent(gptr, inode_id, isdir, key->hfsPlus.parentID, parentid, &key->hfsPlus.nodeName); } /* Compare the names for directory inode only because the names @@ -633,14 +689,24 @@ int inode_check(SGlobPtr gptr, PrimeBuckets *bucket, if ((found_len != calc_len) || (strncmp(calc_name, found_name, calc_len) != 0)) { - (void) record_inode_badname(gptr, inode_id, found_name, - calc_name); + + (void) record_inode_badname(gptr, inode_id, isdir, found_name, calc_name); + + /* Inodes in the "pending delete" state (that were deleted + * while open but the filesystem was ejected before they could + * get removed) will simply be deleted. + * All others will be moved to lost+found (and the other + * properties will be checked in the next recheck). + * So we bail out for now. + */ + retval = 0; + goto out; } } /* At least one hard link should always point at an inode. */ if (linkCount == 0) { - record_link_badchain(gptr, isdir); + record_orphan_inode(gptr, inode_id, isdir); if (fsckGetVerbosity(gptr->context) >= kDebugLog) { plog ("\tlinkCount=0 for dirinode=%u\n", inode_id); } diff --git a/fsck_hfs.tproj/fsck_hfs_msgnums.h b/fsck_hfs.tproj/fsck_hfs_msgnums.h index e56f87e..bb57ddd 100644 --- a/fsck_hfs.tproj/fsck_hfs_msgnums.h +++ b/fsck_hfs.tproj/fsck_hfs_msgnums.h @@ -152,8 +152,8 @@ enum { E_HsFldCount = 581, /* HasFolderCount flag needs to be set */ E_BadPermPrivDir = 582, /* Incorrect permissions for private directory for directory hard links */ E_DirInodeBadFlags = 583, /* Incorrect flags for directory inode */ - E_DirInodeBadParent = -584, /* Invalid parent for directory inode */ - E_DirInodeBadName = -585, /* Invalid name for directory inode */ + E_DirInodeBadParent = 584, /* Invalid parent for directory inode */ + E_DirInodeBadName = 585, /* Invalid name for directory inode */ E_DirHardLinkChain = 586, /* Incorrect number of directory hard link count */ E_DirHardLinkOwnerFlags = 587, /* Incorrect owner flags for directory hard link */ E_DirHardLinkFinderInfo = 588, /* Invalid finder info for directory hard link */ @@ -165,7 +165,7 @@ enum { E_InvalidLinkChainPrev = 593, /* Previous ID in a hard lnk chain is incorrect */ E_InvalidLinkChainNext = 594, /* Next ID in a hard link chain is incorrect */ E_FileInodeBadFlags = 595, /* Incorrecgt flags for file inode */ - E_FileInodeBadParent = -596, /* Invalid parent for file inode */ + E_FileInodeBadParent = 596, /* Invalid parent for file inode */ E_FileInodeBadName = -597, /* Invalid name for file inode */ E_FileHardLinkChain = 598, /* Incorrect number of file hard link count */ E_FileHardLinkFinderInfo= 599, /* Invalid finder info for file hard link */ -- 2.33.0
TestCases.tar.bz2
Description: BZip2 compressed data
