Opening files without metadata EA may result in broken metadata EA

ad_open_hf_ea() calls ad_flush() after initializing the new metadata,
but in ad_flush() ad_flush_ea() wasn't called because we didn't
yet increment the refcount.
Fix this by increasing the refcount in ad_open_hf() before calling
into ad_open_hf_ea(), decrementing it again in case of an error.

This ensure a valid metadata EA is created, but it still doesn't
set the CNID field values. Therefor in afp_openfork() we must check
whether the ad_open() created a metadata EA and then set the CNID
info.

Finally, check for malformed metadata EAs and delete them.

Fixes bug #510.

diff --git a/NEWS b/NEWS
index c6b85fc6008554e71410b14751cbe3f0c6e93c3b..5d6ff9cf8240f415c9d3abb4560c2ff40baed9ec 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,8 @@
 Changes in 3.0.4
 ================
+* FIX: Opening files without metadata EA may result in an invalid
+       metadata EA. Check for malformed metadata EAs and delete them.
+       Fixes bug #510.
 
 Changes in 3.0.3
 ================

diff --git a/etc/afpd/fork.c b/etc/afpd/fork.c
index 4e47b91b92a7ebd96d21987ebd82940e9a7990d5..0d050b3259e30f8a5e818cdcfc972210f1f9a688 100644 (file)
--- a/etc/afpd/fork.c
+++ b/etc/afpd/fork.c
@@ -386,6 +386,16 @@ int afp_openfork(AFPObj *obj _U_, char *ibuf, size_t ibuflen _U_, char *rbuf, si
 
     if (ad_open(ofork->of_ad, upath, adflags, 0666) == 0) {
         ofork->of_flags |= AFPFORK_META;
+        if (ad_get_MD_flags(ofork->of_ad) & O_CREAT) {
+            LOG(log_debug, logtype_afpd, "afp_openfork(\"%s\"): setting CNID", upath);
+            cnid_t id;
+            if ((id = get_id(vol, ofork->of_ad, st, dir->d_did, upath, strlen(upath))) == CNID_INVALID) {
+                LOG(log_error, logtype_afpd, "afp_createfile(\"%s\"): CNID error", upath);
+                goto openfork_err;
+            }
+            (void)ad_setid(ofork->of_ad, st->st_dev, st->st_ino, id, dir->d_did, vol->v_stamp);
+            ad_flush(ofork->of_ad);
+        }
     } else {
         switch (errno) {
         case EACCES:

diff --git a/libatalk/adouble/ad_open.c b/libatalk/adouble/ad_open.c
index a2f6f00e1ca73b027d0b9d8d074058f91b1deb5c..12250055dbe326e6cc30be3777734a56ab58b316 100644 (file)
--- a/libatalk/adouble/ad_open.c
+++ b/libatalk/adouble/ad_open.c
@@ -616,6 +616,7 @@ EC_CLEANUP:
 
 static int ad_header_read_ea(const char *path, struct adouble *ad, const struct stat *hst _U_)
 {
+    EC_INIT;
     uint16_t nentries;
     int      len;
     ssize_t  header_len;
@@ -625,15 +626,15 @@ static int ad_header_read_ea(const char *path, struct adouble *ad, const struct
         header_len = sys_fgetxattr(ad_meta_fileno(ad), AD_EA_META, ad->ad_data, AD_DATASZ_EA);
     else
         header_len = sys_getxattr(path, AD_EA_META, ad->ad_data, AD_DATASZ_EA);
-     if (header_len < 1) {
+    if (header_len < 1) {
         LOG(log_debug, logtype_ad, "ad_header_read_ea: %s", strerror(errno));
-        return -1;
+        EC_FAIL;
     }
 
-    if (header_len < AD_HEADER_LEN) {
-        LOG(log_error, logtype_ad, "ad_header_read_ea(\"%s\"): bogus AppleDouble header.", fullpathname(path));
-        errno = EIO;
-        return -1;
+    if (header_len < AD_DATASZ_EA) {
+        LOG(log_error, logtype_ad, "ad_header_read_ea(\"%s\"): short metadata EA", fullpathname(path));
+        errno = EINVAL;
+        EC_FAIL;
     }
 
     memcpy(&ad->ad_magic, buf, sizeof( ad->ad_magic ));
@@ -644,28 +645,44 @@ static int ad_header_read_ea(const char *path, struct adouble *ad, const struct
 
     if ((ad->ad_magic != AD_MAGIC) || (ad->ad_version != AD_VERSION2)) {
         LOG(log_error, logtype_ad, "ad_header_read_ea(\"%s\"): wrong magic or version", fullpathname(path));
-        errno = EIO;
-        return -1;
+        errno = EINVAL;
+        EC_FAIL;
     }
 
     memcpy(&nentries, buf + ADEDOFF_NENTRIES, sizeof( nentries ));
     nentries = ntohs( nentries );
-
-    /* Protect against bogus nentries */
-    len = nentries * AD_ENTRY_LEN;
-    if (len + AD_HEADER_LEN > sizeof(ad->ad_data))
-        len = sizeof(ad->ad_data) - AD_HEADER_LEN;
-    if (len > header_len - AD_HEADER_LEN) {
-        LOG(log_error, logtype_ad, "ad_header_read_ea(\"%s\"): can't read entry info.", fullpathname(path));
-        errno = EIO;
-        return -1;
+    if (nentries != ADEID_NUM_EA) {
+        LOG(log_error, logtype_ad, "ad_header_read_ea(\"%s\"): invalid number of entries: %d", fullpathname(path), nentries);
+        errno = EINVAL;
+        EC_FAIL;
     }
-    nentries = len / AD_ENTRY_LEN;
 
     /* Now parse entries */
     parse_entries(ad, buf + AD_HEADER_LEN, nentries);
 
-    return 0;
+    if (nentries != ADEID_NUM_EA
+        || !ad_entry(ad, ADEID_FINDERI)
+        || !ad_entry(ad, ADEID_COMMENT)
+        || !ad_entry(ad, ADEID_FILEDATESI)
+        || !ad_entry(ad, ADEID_AFPFILEI)
+        || !ad_entry(ad, ADEID_PRIVDEV)
+        || !ad_entry(ad, ADEID_PRIVINO)
+        || !ad_entry(ad, ADEID_PRIVSYN)
+        || !ad_entry(ad, ADEID_PRIVID)) {
+        LOG(log_error, logtype_ad, "ad_header_read_ea(\"%s\"): invalid metadata EA", fullpathname(path));
+        errno = EINVAL;
+        EC_FAIL;
+    }
+
+EC_CLEANUP:
+    if (ret != 0 && errno == EINVAL) {
+        become_root();
+        (void)sys_removexattr(path, AD_EA_META);
+        unbecome_root();
+        LOG(log_error, logtype_ad, "ad_header_read_ea(\"%s\"): deleted invalid metadata EA", fullpathname(path), nentries);
+        errno = ENOENT;
+    }
+    EC_EXIT;
 }
 
 /*!
@@ -1131,7 +1148,7 @@ static int ad_open_hf(const char *path, int adflags, int mode, struct adouble *a
 {
     int ret = 0;
 
-    memset(ad->ad_eid, 0, sizeof( ad->ad_eid ));
+    ad->ad_meta_refcount++;
 
     switch (ad->ad_vers) {
     case AD_VERSION2:
@@ -1145,10 +1162,10 @@ static int ad_open_hf(const char *path, int adflags, int mode, struct adouble *a
         break;
     }
 
-    if (ret == 0)
-        ad->ad_meta_refcount++;
-    else
+    if (ret != 0) {
+        ad->ad_meta_refcount--;
         ret = ad_error(ad, adflags);
+    }
 
     return ret;
 }