#659 Memory corruption bug in extattr.c : bsd_attr_list()
There's a major memory corruption bug on BSD systems in bsd_attr_list().
At the end, when the list of pascal strings returned by extattr_list_* is converted to a list of C-strings, there are two bugs:
/* Convert from pascal strings to C strings */
len = list[0];
Here, both len and list are signed. If list[0] was 128 (unsigned), it would be sign extended, and len would equal -1. (Note that xattr names on BSD may be up to 255 bytes).
This should be changed to len = (unsigned char)list[0];
And immediately follow that is another bug,
memmove(list, list + 1, list_size);
We're copying 1 too many bytes here. It should copy list_size-1 bytes instead. As written, we're copying 1 byte of garbage past the end of the buffer.
Within the body of the loop, where we set
len = list[i];
we're again subject to erroneous sign extension. This should be changed to
len = (unsigned char)list[i];
I discovered this bug when I was copying a giant dataset from a netatalk share, and one file in particular kept failing to read its xattrs. And the behavior was non-deterministic -- the xattr names would appear corrupted in different ways on different attempts. I enabled maxdebug logging, and discovered this pascal-to-C-string loop iterating dozens of times, printing negative position values.
I'll attach the logs so you can see what I mean.
I found that afpd would crash with a segfault as soon as one particular user connected. I spent a week tracking this down. I wish I'd seen your bug report sooner.
A cursory code inspection led me to believe that this code could not cause any problems. The array "list" is 4k long, so reading one byte further into it should not cause any problems. I did not see the negative values of "i" that you observed, though this would happen if you read a value larger than 127 in the garbage space.
My crash only happens with an optimized (-O1 or higher) binary on FreeBSD 12.
I suspect my particular problem happened because the optimized binary uses an llvm intrinsic to do the memmove, and that this intrinsic does something tricky under the covers.
I have a proposed fix for this in https://sourceforge.net/p/netatalk/code/merge-requests/1/
These patches were merged back in 2021 with these two commits:
https://github.com/Netatalk/netatalk/commit/3114ed028a2486435d911fec9e3a3dfa20d3ea31
https://github.com/Netatalk/netatalk/commit/cc825e86b84e19c38c11a80666fe44cd99c122b8