Bug 14808 - smbc_getxattr() return value is incorrect
Summary: smbc_getxattr() return value is incorrect
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: 4.14.6
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Jule Anger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-23 07:39 UTC by Dmitry Degtyarev
Modified: 2023-10-05 14:15 UTC (History)
6 users (show)

See Also:


Attachments
git-am fix for 4.17.next, 4.16.next. (5.51 KB, patch)
2022-11-01 19:26 UTC, Jeremy Allison
jra: review? (dmulder)
slow: review+
Details
patch for 4.18 (3.73 KB, patch)
2023-09-05 15:53 UTC, Andreas Schneider
jra: review+
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Degtyarev 2021-08-23 07:39:36 UTC
Expected behaviour: "@return 0 on success, < 0 on error" (from function comment).
Actual behaviour: returns positive value on success, < 0 on error.

In the code you can see that getxattr() uses cacl_get():
https://github.com/samba-team/samba/blob/f753e2f7acf8f3394a5f1107344d0323acc05694/source3/libsmb/libsmb_xattr.c#L2169
And then forwards the return value from cacl_get():
https://github.com/samba-team/samba/blob/f753e2f7acf8f3394a5f1107344d0323acc05694/source3/libsmb/libsmb_xattr.c#L2179
Meanwhile, cacl_get() returns a non-zero value called "n_used":
https://github.com/samba-team/samba/blob/f753e2f7acf8f3394a5f1107344d0323acc05694/source3/libsmb/libsmb_xattr.c#L1488

(@Nable80 suggested the bugreport)
Comment 1 Samba QA Contact 2022-11-01 18:32:04 UTC
This bug was referenced in samba master:

74636dfe24c15677261fc40c0a4ec62404898cf4
bdbb38d16c8eaff33484bb747efa639c4d8e7f35
Comment 2 Jeremy Allison 2022-11-01 19:26:34 UTC
Created attachment 17610 [details]
git-am fix for 4.17.next, 4.16.next.

Cherry-picked cleanly from master.
Comment 3 Jeremy Allison 2023-01-13 17:59:25 UTC
Comment on attachment 17610 [details]
git-am fix for 4.17.next, 4.16.next.

Ping David - can we get this into the next release please ?
Comment 4 Ralph Böhme 2023-01-13 18:27:23 UTC
Reassigning to Jule for inclusion in 4.16 and 4.17.
Comment 5 Jule Anger 2023-01-16 09:05:09 UTC
Pushed to autobuild-v4-{17,16}-test.
Comment 6 Samba QA Contact 2023-01-16 10:48:27 UTC
This bug was referenced in samba v4-16-test:

628a1c338277f3fc4250fd54809bf326ec15c0ef
113536e0d735a5235f8be29d4fd1cfc8177930b1
Comment 7 Samba QA Contact 2023-01-16 10:50:03 UTC
This bug was referenced in samba v4-17-test:

a92a0043493f678338ecdefa49810e91f6bb2a1a
50330f69a073915c604070a1889e92af0f0a2006
Comment 8 Jule Anger 2023-01-16 11:54:42 UTC
Closing out bug report.

Thanks!
Comment 9 Samba QA Contact 2023-01-26 17:49:49 UTC
This bug was referenced in samba v4-17-stable (Release samba-4.17.5):

a92a0043493f678338ecdefa49810e91f6bb2a1a
50330f69a073915c604070a1889e92af0f0a2006
Comment 10 Samba QA Contact 2023-02-16 16:36:37 UTC
This bug was referenced in samba v4-16-stable (Release samba-4.16.9):

628a1c338277f3fc4250fd54809bf326ec15c0ef
113536e0d735a5235f8be29d4fd1cfc8177930b1
Comment 11 Volker Lendecke 2023-04-04 10:24:56 UTC
See https://gitlab.com/samba-team/samba/-/merge_requests/3024
Comment 12 Remi Collet 2023-04-04 12:37:08 UTC
The fix in 4.16.9/4.17.5 for this issue introduces a BC break

This was really a documentation issue for the return value

Documentation for size parameter describes the usage
of the returned needed size:

 * @param size      The size of the buffer pointed to by value.  This parameter
 *                  may also be zero, in which case the size of the buffer
 *                  required to hold the attribute value will be returned,
 *                  but nothing will be placed into the value buffer.

Which is also consistent with getxattr syscall:

RETURN VALUE
  On success, these calls return a nonnegative value which is the size (in bytes) of the extended attribute value.
  On failure, -1 is re-turned and errno is set to indicate the error.


In short, the documentation needs to be fixed, not the function behavior (especially in a minor release)
Comment 13 Dmitry Degtyarev 2023-04-04 15:29:00 UTC
Didn't realize there's code that depends on getxattr() returning size. Reverting the fix and changing the documentation instead is definitely the way to go then.
Comment 14 Jeremy Allison 2023-04-04 16:11:28 UTC
Fix looks correct. RB+ by me on the MR. Just needs a second Team reviewer. Thanks for catching this.
Comment 15 Samba QA Contact 2023-04-09 10:45:04 UTC
This bug was referenced in samba master:

4fc166628fda160d1cd38c140a9664defc5844ab
0cd66fe6bd4ac2aa0b302ddf3eb5068fc9c522ec
Comment 16 Remi Collet 2023-04-11 09:53:18 UTC
Please also apply in 4.16, 4.17, 4.18 where the regression appears
Comment 17 Andreas Schneider 2023-09-05 15:53:00 UTC
This is fixed in 4.19 and 4.17, but NOT in 4.18!
Comment 18 Andreas Schneider 2023-09-05 15:53:25 UTC
Created attachment 18078 [details]
patch for 4.18
Comment 19 Jeremy Allison 2023-09-05 17:34:51 UTC
Re-assigning to Jule to update the 4.18.next release.
Comment 20 Jule Anger 2023-09-06 08:35:40 UTC
Pushed to autobuild-v4-18-test.
Comment 21 Samba QA Contact 2023-09-06 09:29:05 UTC
This bug was referenced in samba v4-18-test:

0d8e5ba4f511d7da2c76b01931190fe1d9bd6a7e
5cf6870718cf4356e627e0159866faf53ee11a08
Comment 22 Jule Anger 2023-09-06 09:58:32 UTC
Closing out bug report.

Thanks!
Comment 23 Samba QA Contact 2023-09-27 08:14:48 UTC
This bug was referenced in samba v4-18-stable (Release samba-4.18.7):

0d8e5ba4f511d7da2c76b01931190fe1d9bd6a7e
5cf6870718cf4356e627e0159866faf53ee11a08
Comment 24 Remi Collet 2023-10-05 14:15:07 UTC
Notice that 4.18.7 and 4.19 are OK
but 4.17.11 is still broken