[PATCH 1/3] apfsck: verify the name of an inode's primary link

[Ernesto A. Fernández]

Check that the name of the primary link for an inode is correctly
reported by the inode's extended fields. To keep the patch simple,
focus on inodes with multiple links, for now.

Signed-off-by: Ernesto A. Fernández <ernesto.mn...@gmail.com>
---
apfsck/inode.c | 54 +++++++++++++++++++++++++++++++++++++++++++++---------
apfsck/inode.h | 1 +
2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/apfsck/inode.c b/apfsck/inode.c
index 8772083..b0698c3 100644
--- a/apfsck/inode.c
+++ b/apfsck/inode.c
@@ -61,18 +61,31 @@ struct inode **alloc_inode_table(void)
}

/**
- * free_sibling_links - Free the sibling links for an inode
+ * free_inode_names - Free all data on an inode's names
* @inode: inode to free
*
- * Also checks that the number of listed siblings is correct, and that all
- * of them had both a record and a dentry xfield.
+ * Frees the primary name and all sibling links, but not before running a few
+ * remaining consistency checks.
+ * remaining checking that
+ * the number of listed siblings is correct, that all of them had both a record
+ * and a dentry xfield, and that the primary link matches the primary name.
*/
-static void free_sibling_links(struct inode *inode)
+static void free_inode_names(struct inode *inode)
{
struct sibling *current = inode->i_siblings;
struct sibling *next;
u32 count = 0;

+ /* The primary link has the lowest id, so it's the first in the list */
+ if (current) {
+ if (!inode->i_name)
+ report("Inode record", "no name for primary link.");
+ if (strcmp(inode->i_name, (char *)current->s_name))
+ report("Inode record", "wrong name for primary link.");
+ }
+ free(inode->i_name);
+ inode->i_name = NULL;
+
while (current) {
if (!current->s_checked)
report("Catalog", "orphaned or missing sibling link.");
@@ -108,7 +121,7 @@ void free_inode_table(struct inode **table)
current = table[i];
while (current) {
check_inode_stats(current);
- free_sibling_links(current);
+ free_inode_names(current);

next = current->i_next;
free(current);
@@ -232,6 +245,32 @@ static int read_rdev_xfield(char *xval, int len, struct inode *inode)
}

/**
+ * read_name_xfield - Parse a name xfield and check its consistency
+ * @xval: pointer to the xfield value
+ * @len: remaining length of the inode value
+ * @inode: struct to receive the results
+ *
+ * Returns the length of the xfield value.
+ */
+static int read_name_xfield(char *xval, int len, struct inode *inode)
+{
+ int xlen;
+
+ xlen = strnlen(xval, len - 1) + 1;
+ if (xval[xlen - 1] != 0)
+ report("Name xfield", "name with no null termination");
+
+ inode->i_name = malloc(xlen);
+ if (!inode->i_name) {
+ perror(NULL);
+ exit(1);
+ }
+ strcpy(inode->i_name, xval);
+
+ return xlen;
+}
+
+/**
* read_dstream_xfield - Parse a dstream xfield and check its consistency
* @xval: pointer to the xfield value
* @len: remaining length of the inode value
@@ -316,10 +355,7 @@ static void parse_inode_xfields(struct apfs_xf_blob *xblob, int len,
xlen = read_rdev_xfield(xval, len, inode);
break;
case APFS_INO_EXT_TYPE_NAME:
- xlen = strnlen(xval, len - 1) + 1;
- if (xval[xlen - 1] != 0)
- report("Inode xfield",
- "name with no null termination");
+ xlen = read_name_xfield(xval, len, inode);
break;
case APFS_INO_EXT_TYPE_DSTREAM:
xlen = read_dstream_xfield(xval, len, inode);
diff --git a/apfsck/inode.h b/apfsck/inode.h
index ba09eb2..102484c 100644
--- a/apfsck/inode.h
+++ b/apfsck/inode.h
@@ -157,6 +157,7 @@ struct inode {
u64 i_alloced_size; /* Inode size, including unused */
u64 i_sparse_bytes; /* Number of sparse bytes */
u32 i_rdev; /* Device ID */
+ char *i_name; /* Name of primary link */

/* Inode stats measured by the fsck */
u32 i_child_count; /* Number of children of directory */
--
2.11.0

[Ernesto A. Fernández]

In contradiction with the official documentation, it seems that inodes
with a single link are also required to report their name in an xfield.
Adding a check for this is somewhat harder than may seem, because these
inodes don't always keep a list of sibling links. Save the name of the
first dentry encountered for the inode, and use that if no other links
show up.

Signed-off-by: Ernesto A. Fernández <ernesto.mn...@gmail.com>
---

apfsck/dir.c | 13 +++++++++++++
apfsck/inode.c | 15 ++++++++++++---
apfsck/inode.h | 1 +
3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/apfsck/dir.c b/apfsck/dir.c
index b70027e..eaaa7e9 100644
--- a/apfsck/dir.c
+++ b/apfsck/dir.c
@@ -4,6 +4,9 @@
* Copyright (C) 2018 Ernesto A. Fernández <ernesto.mn...@gmail.com>
*/

+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
#include "apfsck.h"
#include "dir.h"
#include "inode.h"
@@ -102,6 +105,16 @@ void parse_dentry_record(struct apfs_drec_hashed_key *key,
inode = get_inode(ino, vsb->v_inode_table);
inode->i_link_count++;

+ if (!inode->i_first_name) {
+ /* No dentry for this inode has been seen before */
+ inode->i_first_name = malloc(namelen);
+ if (!inode->i_first_name) {

+ perror(NULL);
+ exit(1);
+ }

+ strcpy(inode->i_first_name, (char *)key->name);
+ }
+
parent_ino = cat_cnid(&key->hdr);
check_inode_ids(ino, parent_ino);
if (parent_ino != APFS_ROOT_DIR_PARENT) {
diff --git a/apfsck/inode.c b/apfsck/inode.c
index b0698c3..a0775f5 100644
--- a/apfsck/inode.c
+++ b/apfsck/inode.c
@@ -76,15 +76,24 @@ static void free_inode_names(struct inode *inode)

struct sibling *next;
u32 count = 0;

- /* The primary link has the lowest id, so it's the first in the list */
+ if (!inode->i_name) /* Oddly, this seems to be always required */

+ report("Inode record", "no name for primary link.");

+ if (!inode->i_first_name)
+ report("Catalog", "inode with no dentries.");
+
if (current) {
- if (!inode->i_name)
- report("Inode record", "no name for primary link.");
+ /* Primary link has lowest id, so it comes first in the list */

if (strcmp(inode->i_name, (char *)current->s_name))

report("Inode record", "wrong name for primary link.");

+ } else {
+ /* No siblings, so the primary link is the first and only */
+ if (strcmp(inode->i_name, inode->i_first_name))
+ report("Inode record", "wrong name for only link.");
}
free(inode->i_name);
inode->i_name = NULL;
+ free(inode->i_first_name);
+ inode->i_first_name = NULL;

while (current) {
if (!current->s_checked)

diff --git a/apfsck/inode.h b/apfsck/inode.h
index 102484c..c04515a 100644
--- a/apfsck/inode.h
+++ b/apfsck/inode.h
@@ -162,6 +162,7 @@ struct inode {

/* Inode stats measured by the fsck */
u32 i_child_count; /* Number of children of directory */

u32 i_link_count; /* Number of dentries for file */
+ char *i_first_name; /* Name of first dentry encountered */
struct sibling *i_siblings; /* Linked list of siblings for inode */

struct inode *i_next; /* Next inode in linked list */
--
2.11.0

[Ernesto A. Fernández]

It seems that sibling ids come from the same pool as inode numbers, so
values under APFS_MIN_USER_INO_NUM should not be in use here either.

Signed-off-by: Ernesto A. Fernández <ernesto.mn...@gmail.com>
---

apfsck/inode.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/apfsck/inode.c b/apfsck/inode.c
index a0775f5..3e844d7 100644
--- a/apfsck/inode.c
+++ b/apfsck/inode.c
@@ -608,6 +608,11 @@ void parse_sibling_record(struct apfs_sibling_link_key *key,
report("Sibling link record", "inode is missing");

sibling = get_sibling(le64_to_cpu(key->sibling_id), namelen, inode);
+
+ /* It seems that sibling ids come from the same pool as inode numbers */
+ if (sibling->s_id < APFS_MIN_USER_INO_NUM)
+ report("Sibling record", "invalid sibling id.");
+
set_or_check_sibling(le64_to_cpu(val->parent_id), namelen, val->name,
sibling);
}
--
2.11.0