Attributes not correctly copied (caused by fix to ticket #4015)

[someguy]

File attributes are not copied correctly when using cp from the terminal. The problem is caused by a bug introduced by ticket #4015 (hrev31000). A change was made to cp in order to copy empty attributes in files. The fix has a bug in it however.

Line 166 in haiku/trunk/src/bin/coreutils/src/copy.c currently reads:

if (bytesWritten != bytesRead

bytesRead == 0)

It should read:

if (bytesWritten == bytesRead

bytesRead == 0)

The way it currently is, it continues through the loop one more time which writes 0 bytes to the attribute which erases the bits written the first time through.

The bug can be reproduced simply by copying any file with attributes from the terminal using cp. You'll find the copied file has all the attributes of the original file, but they are empty. My privately built cp with the change fixes the problem.

[stippi]

Your proposed fix would definitely be wrong. Consider the case that the attribute does not fit into the temporary buffer used in the loop. Those attributes would all only get their first chunk copied. In fact, the loop looks just fine as it is, I am trying to understand what the problem is... It looks a bit as if writing 0 bytes behind an existing attribute has some effect, while it shouldn't.

[stippi]

The problem is actually in the fs_write_attr() implementation. It will always create and truncate the existing attribute. The pos argument is irrelevant. The cp loop would iterate one more time unnecessarily and try to read/write 0 bytes, but that should not have been a problem. I will commit a better loop to cp after I fixed the fs_write_attr() implementation.

[stippi]

With hrev31310, the problem is fixed. Thanks a lot for the pointer in the right direction!

[someguy]

Thanks Stippi!!

I came to my fix by reading the docs on fs_write_attr(). It says that it is an incomplete implementation and that pos is supposed to be ignored. It also says that you can basically write to an attribute only once as the next write will completely replace the attribute contents, even if the old data is larger than the new. In the case of the loop, it should actually never go through the loop more than once.

If you change this behavior in fw_write_attr(), you could break a lot of existing apps.

[anevilyak]

Replying to someguy:

If you change this behavior in fw_write_attr(), you could break a lot of existing apps.

That seems unlikely, the majority of apps do it via BNode/BFile's WriteAttr call, comparatively few use the low level C API directly.

[stippi]

It wouldn't be good to change the behavior even if there was only a slim chance of breaking any app. However, I changed it such that writing chunks in a loop is now possible, but passing an offset of 0 will truncate any existing attribute of the same name. This means that only apps which passed an uninitialized pos argument (with a random value) could be broken now. I guess taking that risk is acceptable. :-)

In any case it's important that writing attributes in chunks is possible, otherwise you limit the maximum argument length drastically, since it would otherwise need to fit into a single allocation. As in the example of the cp copy loop, it wouldn't support attributes larger than 64K.

[mmlr]

Regarding the copy mechanism I still don't see why cp uses a different implementation than copyattr. IMO it would make sense to sync those two to make sure they both behave the same and are correct.