Skip to content

Fix handling of objects with many xattrs on FreeBSD #766

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tridge merged 1 commit into from
Aug 23, 2025
Merged

Fix handling of objects with many xattrs on FreeBSD #766

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions lib/sysxattrs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,17 +126,26 @@ ssize_t sys_llistxattr(const char *path, char *list, size_t size)
unsigned char keylen;
ssize_t off, len = extattr_list_link(path, EXTATTR_NAMESPACE_USER, list, size);
Copy link
Contributor

@rosorio rosorio Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Called this way extattr_list_link(2) behaves like read(2) and return the amount of bytes there was able to write in the list buffer which is not that informative and can misleading if list has the required size.

I suggest to perform a call of extattr_list_link(2) with a null pointer which returns us the exact amount of bytes required in the buffer without perform any write.

       ssize_t off = 0, len;

       /* Check for the required size to store the extended attibutes  */
       len = extattr_list_link(path, EXTATTR_NAMESPACE_USER, NULL, size);

Copy link
Contributor Author

@ptrrkssn ptrrkssn Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and no. Doing it that way has a potential to introduce a race condition if someone changes the extattr in the (very short) time between then call with NULL was done and when you actually do the real read. Doing it the way it's done now removes that race.

Copy link
Contributor

@rosorio rosorio Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ask myself the same question when doing my tests.
But if you consider that rsync process extended attributes by

  • extracting xattrs key list
  • then in a second time for each key, recovering the associated value
    the race condition you describe can occurs at any time (eg. if you start removing xattrs when rsync is working)

If you are looking for full consistent remote copy tool, rsync only is probably not the most appropriate tool and you should consider make zfs snapshots, mount them then use rsync for the file transfer.

In fact, in worst case scenario, we can have a truncated key value which makes a particular file transfer fail because the value associated with the broken key is missing. But I don't expect any crash.


if (len <= 0 || (size_t)len > size)
if (len <= 0 || size == 0)
return len;

if ((size_t)len >= size) {
Copy link
Contributor

@rosorio rosorio Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The equal test is not required anymore just do if ((size_t)len > size)

Copy link
Contributor Author

@ptrrkssn ptrrkssn Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that if extattr_list_link(..., size) returns len==size then we don't know if we got all the data or if there is more data to read. It might be exactly 'size' bytes that was available there, or it might be more, but since extattr_list_link() will return just 'size' in both cases then that will fail. Now the code after that which parses the array might return EINVAL if we just got a partial buffer - but there is a small chance that the list of extattrs is so perfectly aligned that it terminates exactly at the 'size' border - but there are still more extattrs that could have been read if the buffer was bigger...

/* FreeBSD extattr_list_xx() returns 'size' as 'len' in case there are
Copy link
Contributor

@rosorio rosorio Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make the comment simpler : "buffer is too small, behave as linux and request a bigger buffer"

more data available, truncating the output, we solve this by signalling
ERANGE in case len == size so that the code in xattrs.c will retry with
a bigger buffer */
errno = ERANGE;
return -1;
}

Copy link
Contributor

@rosorio rosorio Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can perform the real call to retrieve the buffer:

len = extattr_list_link(path, EXTATTR_NAMESPACE_USER, list, size);

if (len <= 0)
    return len;

Edit: Catch an invalid memory access error

/* FreeBSD puts a single-byte length before each string, with no '\0'
* terminator. We need to change this into a series of null-terminted
* strings. Since the size is the same, we can simply transform the
* output in place. */
for (off = 0; off < len; off += keylen + 1) {
Copy link
Contributor

@rosorio rosorio Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This for loop is quite inefficient performing multiple memmove(2) calls, what about doing it one:

       keylen = (unsigned char)list[0];
       memmove(list, list+1, len-1);
       list[len-1] = '\0';
       for (off = keylen; off < (len - 1); off += (keylen + 1)) {
               keylen = (unsigned char)list[off];
               list[off] = '\0';
        }

        if ((size_t)len != (size_t)(off+1)) {
                errno = ERANGE;
                return -1;
        }

Edited: Added a final test to ensure second extattr_list_link didn't truncate the list due to a change of the file extended attribute list (eg. a new attribute was added). On failure we request a longer buffer.

Copy link
Contributor Author

@ptrrkssn ptrrkssn Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like a good suggestion, I never looked at that part of the code :-).

However, isn't the list[len-1] = '\0' overwriting the next keylen before you read it?

Copy link
Contributor

@rosorio rosorio Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because we did the memmove before changing list[len-1]

before memmove list = [0x1,'A',0x1,'B',0x1,'C']
after memmove list = ['A',0x1,'B',0x1,'C','C']
after list[5] = '\0' list = ['A',0x1,'B',0x1,'C','\0']

keylen = ((unsigned char*)list)[off];
if (off + keylen >= len) {
/* Should be impossible, but kernel bugs happen! */
/* Should be impossible, but bugs happen! */
errno = EINVAL;
return -1;
}
Expand Down