[PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Brian Inglis]

[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]

On Tue, 30 May 2023 13:25:38 +0200, Philippe Cerfon wrote:
> Hey there.
> 
> Linux exports XATTR_{NAME,SIZE,LIST}_MAX in it's linux/limits.h and
> e.g. the CPython interpreter uses them for it's XATTRs functions.
> 
> I made a corresponding PR at CPython
> https://github.com/python/cpython/pull/105075 to get the code built
> for Cygwin, but right now this would fail due to the missing
> XATTR_*_MAX symbols.
> 
> The attached patch below would add them to cygwin/limits.h.

Patches for Cygwin under winsup are submitted to cygwin-patches@cygwin.com 
(forwarded there).

> But beware, I'm absolutely no Windows/Cygwin expert ^^ - so whether
> the values I've chosen are actually correct, is more guesswork rather
> than definite knowledge.
> 
> As written in the commit message, I think:
> - XATTR_NAME_MAX corresponds to MAX_EA_NAME_LEN
> and
> - XATTR_SIZE_MAX to MAX_EA_VALUE_LEN
> 
> though I have no idea, whether these are just lower boundaries used by
> Cygwin, while e.g. Windows itself might set longer names or value
> lenghts, and thus - when Cygwin would try to read such - it might get
> into troubles (or rather e.g. CPython, as it's buffers wouldn't
> suffice to read the EA respectively XATTR.
> 
> Neither to i have an idea about XATTR_LIST_MAX. I'm not even 100% sure
> what it means (I guess the max number of XATTRs per file). Not to
> speak about whether there's such maximum for Windows EAs,
> And again - as above - what would happen if Windows itself would set
> more than that limit and within Cygwin one would try to read/list all.
> 
> Thanks,
> Philippe

-- 
Take care. Thanks, Brian Inglis              Calgary, Alberta, Canada

[-- Attachment #2: 0001-export-XATTR_-NAME-SIZE-LIST-_MAX.patch --]
[-- Type: text/x-patch, Size: 1250 bytes --]

From 824bcdf053bfb86570c7edda3c018626dc857a8b Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 30 May 2023 13:16:18 +0200
Subject: [PATCH] export XATTR_{NAME,SIZE,LIST}_MAX

These are used for example by CPython. XATTR_NAME_MAX should correspond to
MAX_EA_NAME_LEN and XATTR_SIZE_MAX to MAX_EA_VALUE_LEN.

It's unclear whether Windows imposes a maximum number of EA's per file and which
value should be used for XATTR_LIST_MAX, so for now Linux' value.

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
---
 winsup/cygwin/include/cygwin/limits.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/winsup/cygwin/include/cygwin/limits.h b/winsup/cygwin/include/cygwin/limits.h
index aefc7c7bd..939ab4f38 100644
--- a/winsup/cygwin/include/cygwin/limits.h
+++ b/winsup/cygwin/include/cygwin/limits.h
@@ -56,4 +56,11 @@ details. */
 #define __PATH_MAX 4096
 #define __PIPE_BUF 4096
 
+/* Keep in sync with MAX_EA_NAME_LEN respectively MAX_EA_VALUE_LEN in
+ * winsup/cygwin/ntea.cc but don not use values that exceed their Linux
+ * counterparts as defined in linux/limits.h. */
+#define XATTR_NAME_MAX 255
+#define XATTR_SIZE_MAX 65536
+#define XATTR_LIST_MAX 65536
+
 #endif /* _CYGWIN_LIMITS_H__ */
-- 
2.40.1

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Corinna Vinschen]

[dropping newlib from the recipient list]

Hi Philippe,

On May 30 14:04, Brian Inglis wrote:
> On Tue, 30 May 2023 13:25:38 +0200, Philippe Cerfon wrote:
> > Hey there.
> > 
> > Linux exports XATTR_{NAME,SIZE,LIST}_MAX in it's linux/limits.h and
> > e.g. the CPython interpreter uses them for it's XATTRs functions.
> > 
> > I made a corresponding PR at CPython
> > https://github.com/python/cpython/pull/105075 to get the code built
> > for Cygwin, but right now this would fail due to the missing
> > XATTR_*_MAX symbols.
> > 
> > The attached patch below would add them to cygwin/limits.h.
> 
> Patches for Cygwin under winsup are submitted to cygwin-patches@cygwin.com
> (forwarded there).
> 
> > But beware, I'm absolutely no Windows/Cygwin expert ^^ - so whether
> > the values I've chosen are actually correct, is more guesswork rather
> > than definite knowledge.
> > 
> > As written in the commit message, I think:
> > - XATTR_NAME_MAX corresponds to MAX_EA_NAME_LEN
> > and
> > - XATTR_SIZE_MAX to MAX_EA_VALUE_LEN
> > 
> > though I have no idea, whether these are just lower boundaries used by
> > Cygwin, while e.g. Windows itself might set longer names or value
> > lenghts, and thus - when Cygwin would try to read such - it might get
> > into troubles (or rather e.g. CPython, as it's buffers wouldn't
> > suffice to read the EA respectively XATTR.

As on Linux, these are upper bounds of the kernel API. In case of
MAX_EA_VALUE_LEN that's especially true, because the defined API
isn't overly consistent: the datastructure allows bigger values
than the function calls are able to handle.  However, file systems
may impose lower limits without them having matching macros.
IIRC, an EA on ext4 may be only 4K, but I'm not entirely sure.

Either way, we can do what you propose, but...

- The XATTR_NAME_MAX value is incorrect. The MAX_EA_NAME_LEN value
  is an internal definition for a name *including* the trailing \0,
  the XATTR_NAME_MAX value defines the max name length *excluding*
  the trailing \0.  Compare with NAME_MAX.

- Whatever that's good for, we actually allow bigger values right
  now.  For compat reasons we only allow attributes starting with
  the "user." prefix, and the *trailing* part after "user." is
  allowed to be 255 bytes long, because we don't store the "user."
  prefix in the EA name on disk.  So in fact, XATTR_NAME_MAX should
  be 255 + strlen("user.") == 260.

- If we actually define these values in limits.h, it would also be a
  good idea to use them in ntea.cc and to throw away the MAX_EA_*_LEN
  macros.

A patch along these lines is appreciated.


Thanks,
Corinna

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Philippe Cerfon]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Hey Corinna, et al.

On Mon, Jun 5, 2023 at 9:05 PM Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> - Whatever that's good for, we actually allow bigger values right
>   now.  For compat reasons we only allow attributes starting with
>   the "user." prefix, and the *trailing* part after "user." is
>   allowed to be 255 bytes long, because we don't store the "user."
>   prefix in the EA name on disk.  So in fact, XATTR_NAME_MAX should
>   be 255 + strlen("user.") == 260.

I haven't given to much though into that right now (just about to go
for 2 weeks on vacation), but if "we" (Cygwin) allow now names up to
260 bytes, because we don't store the "user." .. doesn't that mean
users could set XATTRs, that in the end couldn't be read by e.g. Linux
(should there be, or ever be in the future, support for reading
FAT/NTFS' EAs as XATTRs.... e.g. from the Linux FAT/NTFS fs drivers)?


> - If we actually define these values in limits.h, it would also be a
>   good idea to use them in ntea.cc and to throw away the MAX_EA_*_LEN
>   macros.

Done so in a 2nd commit.
But that commit, right now, really just replaces the name!
MAX_EA_NAME_LEN was set 256, so presumably with the null terminator...
while now it would be set to 260, which seems wrong.

Please just adapt if necessary,... or at least I won't likely be able
to update the patch until in about 2 weeks or so.


Thanks,
Philippe

[-- Attachment #2: 0002-Cygwin-use-new-XATTR_-NAME-SIZE-_MAX-instead-of-MAX_.patch --]
[-- Type: text/x-patch, Size: 3022 bytes --]

From a860212533b2c438832ea419fc23537d05ea2210 Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 6 Jun 2023 02:52:49 +0200
Subject: [PATCH 2/2] Cygwin: use new XATTR_{NAME,SIZE}_MAX instead of
 MAX_EA_{NAME,VALUE}_LEN

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
---
 winsup/cygwin/ntea.cc | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/winsup/cygwin/ntea.cc b/winsup/cygwin/ntea.cc
index a400fcb2b..aafecde59 100644
--- a/winsup/cygwin/ntea.cc
+++ b/winsup/cygwin/ntea.cc
@@ -17,9 +17,7 @@ details. */
 #include "tls_pbuf.h"
 #include <stdlib.h>
 #include <attr/xattr.h>
-
-#define MAX_EA_NAME_LEN    256
-#define MAX_EA_VALUE_LEN 65536
+#include <cygwin/limits.h>
 
 /* At least one maximum sized entry fits.
    CV 2014-04-04: NtQueryEaFile function chokes on buffers bigger than 64K
@@ -27,13 +25,13 @@ details. */
 		  on a remote share, at least on Windows 7 and later.
 		  In theory the buffer should have a size of
 
-		    sizeof (FILE_FULL_EA_INFORMATION) + MAX_EA_NAME_LEN
-		    + MAX_EA_VALUE_LEN
+		    sizeof (FILE_FULL_EA_INFORMATION) + XATTR_NAME_MAX
+		    + XATTR_SIZE_MAX
 
 		  (65804 bytes), but we're opting for simplicity here, and
 		  a 64K buffer has the advantage that we can use a tmp_pathbuf
 		  buffer, rather than having to alloca 64K from stack. */
-#define EA_BUFSIZ MAX_EA_VALUE_LEN
+#define EA_BUFSIZ XATTR_SIZE_MAX
 
 #define NEXT_FEA(p) ((PFILE_FULL_EA_INFORMATION) (p->NextEntryOffset \
 		     ? (char *) p + p->NextEntryOffset : NULL))
@@ -55,7 +53,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
      returns the last EA entry of the file infinitely.  Even utilizing the
      optional EaIndex only helps marginally.  If you use that, the last
      EA in the file is returned twice. */
-  char lastname[MAX_EA_NAME_LEN];
+  char lastname[XATTR_NAME_MAX];
 
   __try
     {
@@ -95,7 +93,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
 	      __leave;
 	    }
 
-	  if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
+	  if ((nlen = strlen (name)) >= XATTR_NAME_MAX)
 	    {
 	      set_errno (EINVAL);
 	      __leave;
@@ -197,7 +195,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
 		  /* For compatibility with Linux, we always prepend "user." to
 		     the attribute name, so effectively we only support user
 		     attributes from a application point of view. */
-		  char tmpbuf[MAX_EA_NAME_LEN * 2];
+		  char tmpbuf[XATTR_NAME_MAX * 2];
 		  char *tp = stpcpy (tmpbuf, "user.");
 		  stpcpy (tp, fea->EaName);
 		  /* NTFS stores all EA names in uppercase unfortunately.  To
@@ -297,7 +295,7 @@ write_ea (HANDLE hdl, path_conv &pc, const char *name, const char *value,
       /* Skip "user." prefix. */
       name += 5;
 
-      if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
+      if ((nlen = strlen (name)) >= XATTR_NAME_MAX)
 	{
 	  set_errno (EINVAL);
 	  __leave;
-- 
2.40.1


[-- Attachment #3: 0001-Cygwin-export-XATTR_-NAME-SIZE-LIST-_MAX.patch --]
[-- Type: text/x-patch, Size: 1099 bytes --]

From b64b9a48c77326ed2544e51422adbe1f1c631542 Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 30 May 2023 13:16:18 +0200
Subject: [PATCH 1/2] Cygwin: export XATTR_{NAME,SIZE,LIST}_MAX

These are used for example by CPython.

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/include/cygwin/limits.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/winsup/cygwin/include/cygwin/limits.h b/winsup/cygwin/include/cygwin/limits.h
index aefc7c7bd..ea3e2836a 100644
--- a/winsup/cygwin/include/cygwin/limits.h
+++ b/winsup/cygwin/include/cygwin/limits.h
@@ -56,4 +56,11 @@ details. */
 #define __PATH_MAX 4096
 #define __PIPE_BUF 4096
 
+/* XATTR_NAME_MAX is the maximum XATTR name length excluding the null
+ * terminator. Since only XATTRs in the `user' namespace are allowed and the
+ * `user.' prefix is not stored, the maximum is increased by 5. */
+#define XATTR_NAME_MAX 260
+#define XATTR_SIZE_MAX 65536
+#define XATTR_LIST_MAX 65536
+
 #endif /* _CYGWIN_LIMITS_H__ */
-- 
2.40.1

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Philippe Cerfon]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Hey Corinna, et al.

On Mon, Jun 5, 2023 at 9:05 PM Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> - Whatever that's good for, we actually allow bigger values right
>   now.  For compat reasons we only allow attributes starting with
>   the "user." prefix, and the *trailing* part after "user." is
>   allowed to be 255 bytes long, because we don't store the "user."
>   prefix in the EA name on disk.  So in fact, XATTR_NAME_MAX should
>   be 255 + strlen("user.") == 260.

I haven't given to much though into that right now (just about to go
for 2 weeks on vacation), but if "we" (Cygwin) allow now names up to
260 bytes, because we don't store the "user." .. doesn't that mean
users could set XATTRs, that in the end couldn't be read by e.g. Linux
(should there be, or ever be in the future, support for reading
FAT/NTFS' EAs as XATTRs.... e.g. from the Linux FAT/NTFS fs drivers)?


> - If we actually define these values in limits.h, it would also be a
>   good idea to use them in ntea.cc and to throw away the MAX_EA_*_LEN
>   macros.

Done so in a 2nd commit.
But that commit, right now, really just replaces the name!
MAX_EA_NAME_LEN was set 256, so presumably with the null terminator...
while now it would be set to 260, which seems wrong.

Please just adapt if necessary,... or at least I won't likely be able
to update the patch until in about 2 weeks or so.


Thanks,
Philippe

[-- Attachment #2: 0001-Cygwin-export-XATTR_-NAME-SIZE-LIST-_MAX.patch --]
[-- Type: text/x-patch, Size: 1099 bytes --]

From b64b9a48c77326ed2544e51422adbe1f1c631542 Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 30 May 2023 13:16:18 +0200
Subject: [PATCH 1/2] Cygwin: export XATTR_{NAME,SIZE,LIST}_MAX

These are used for example by CPython.

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/include/cygwin/limits.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/winsup/cygwin/include/cygwin/limits.h b/winsup/cygwin/include/cygwin/limits.h
index aefc7c7bd..ea3e2836a 100644
--- a/winsup/cygwin/include/cygwin/limits.h
+++ b/winsup/cygwin/include/cygwin/limits.h
@@ -56,4 +56,11 @@ details. */
 #define __PATH_MAX 4096
 #define __PIPE_BUF 4096
 
+/* XATTR_NAME_MAX is the maximum XATTR name length excluding the null
+ * terminator. Since only XATTRs in the `user' namespace are allowed and the
+ * `user.' prefix is not stored, the maximum is increased by 5. */
+#define XATTR_NAME_MAX 260
+#define XATTR_SIZE_MAX 65536
+#define XATTR_LIST_MAX 65536
+
 #endif /* _CYGWIN_LIMITS_H__ */
-- 
2.40.1


[-- Attachment #3: 0002-Cygwin-use-new-XATTR_-NAME-SIZE-_MAX-instead-of-MAX_.patch --]
[-- Type: text/x-patch, Size: 3022 bytes --]

From a860212533b2c438832ea419fc23537d05ea2210 Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 6 Jun 2023 02:52:49 +0200
Subject: [PATCH 2/2] Cygwin: use new XATTR_{NAME,SIZE}_MAX instead of
 MAX_EA_{NAME,VALUE}_LEN

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
---
 winsup/cygwin/ntea.cc | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/winsup/cygwin/ntea.cc b/winsup/cygwin/ntea.cc
index a400fcb2b..aafecde59 100644
--- a/winsup/cygwin/ntea.cc
+++ b/winsup/cygwin/ntea.cc
@@ -17,9 +17,7 @@ details. */
 #include "tls_pbuf.h"
 #include <stdlib.h>
 #include <attr/xattr.h>
-
-#define MAX_EA_NAME_LEN    256
-#define MAX_EA_VALUE_LEN 65536
+#include <cygwin/limits.h>
 
 /* At least one maximum sized entry fits.
    CV 2014-04-04: NtQueryEaFile function chokes on buffers bigger than 64K
@@ -27,13 +25,13 @@ details. */
 		  on a remote share, at least on Windows 7 and later.
 		  In theory the buffer should have a size of
 
-		    sizeof (FILE_FULL_EA_INFORMATION) + MAX_EA_NAME_LEN
-		    + MAX_EA_VALUE_LEN
+		    sizeof (FILE_FULL_EA_INFORMATION) + XATTR_NAME_MAX
+		    + XATTR_SIZE_MAX
 
 		  (65804 bytes), but we're opting for simplicity here, and
 		  a 64K buffer has the advantage that we can use a tmp_pathbuf
 		  buffer, rather than having to alloca 64K from stack. */
-#define EA_BUFSIZ MAX_EA_VALUE_LEN
+#define EA_BUFSIZ XATTR_SIZE_MAX
 
 #define NEXT_FEA(p) ((PFILE_FULL_EA_INFORMATION) (p->NextEntryOffset \
 		     ? (char *) p + p->NextEntryOffset : NULL))
@@ -55,7 +53,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
      returns the last EA entry of the file infinitely.  Even utilizing the
      optional EaIndex only helps marginally.  If you use that, the last
      EA in the file is returned twice. */
-  char lastname[MAX_EA_NAME_LEN];
+  char lastname[XATTR_NAME_MAX];
 
   __try
     {
@@ -95,7 +93,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
 	      __leave;
 	    }
 
-	  if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
+	  if ((nlen = strlen (name)) >= XATTR_NAME_MAX)
 	    {
 	      set_errno (EINVAL);
 	      __leave;
@@ -197,7 +195,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
 		  /* For compatibility with Linux, we always prepend "user." to
 		     the attribute name, so effectively we only support user
 		     attributes from a application point of view. */
-		  char tmpbuf[MAX_EA_NAME_LEN * 2];
+		  char tmpbuf[XATTR_NAME_MAX * 2];
 		  char *tp = stpcpy (tmpbuf, "user.");
 		  stpcpy (tp, fea->EaName);
 		  /* NTFS stores all EA names in uppercase unfortunately.  To
@@ -297,7 +295,7 @@ write_ea (HANDLE hdl, path_conv &pc, const char *name, const char *value,
       /* Skip "user." prefix. */
       name += 5;
 
-      if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
+      if ((nlen = strlen (name)) >= XATTR_NAME_MAX)
 	{
 	  set_errno (EINVAL);
 	  __leave;
-- 
2.40.1

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Corinna Vinschen]

Hi Philippe,

On Jun  6 03:14, Philippe Cerfon wrote:
> Hey Corinna, et al.
> 
> On Mon, Jun 5, 2023 at 9:05 PM Corinna Vinschen
> <corinna-cygwin@cygwin.com> wrote:
> > - Whatever that's good for, we actually allow bigger values right
> >   now.  For compat reasons we only allow attributes starting with
> >   the "user." prefix, and the *trailing* part after "user." is
> >   allowed to be 255 bytes long, because we don't store the "user."
> >   prefix in the EA name on disk.  So in fact, XATTR_NAME_MAX should
> >   be 255 + strlen("user.") == 260.
> 
> I haven't given to much though into that right now (just about to go
> for 2 weeks on vacation), but if "we" (Cygwin) allow now names up to
> 260 bytes, because we don't store the "user." .. doesn't that mean
> users could set XATTRs, that in the end couldn't be read by e.g. Linux
> (should there be, or ever be in the future, support for reading
> FAT/NTFS' EAs as XATTRs.... e.g. from the Linux FAT/NTFS fs drivers)?

No, that's no problem.  The names on disk are actually just 255 chars
long, so the Linux kernel will handle them just fine.  It's only Cygwin
which should expose 260 chars to user space to emulate the "user."
prefix.

> > - If we actually define these values in limits.h, it would also be a
> >   good idea to use them in ntea.cc and to throw away the MAX_EA_*_LEN
> >   macros.
> 
> Done so in a 2nd commit.
> But that commit, right now, really just replaces the name!
> MAX_EA_NAME_LEN was set 256, so presumably with the null terminator...
> while now it would be set to 260, which seems wrong.

Yes, it is wrong, because the value of MAX_EA_NAME_LEN / XATTR_NAME_MAX
is used for array size definitions as well as comparisons.

> Please just adapt if necessary,... or at least I won't likely be able
> to update the patch until in about 2 weeks or so.

No worries, we're in no hurry.


Thanks,
Corinna

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Philippe Cerfon]

On Tue, Jun 6, 2023 at 3:28 PM Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> Yes, it is wrong, because the value of MAX_EA_NAME_LEN / XATTR_NAME_MAX
> is used for array size definitions as well as comparisons.

Do you prefer to keep MAX_EA_NAME_LEN and just have that set like,
perhaps with an additional comment that explains it:
   #define MAX_EA_NAME_LEN    (XATTR_NAME_MAX + 1 - strlen("user."))
or rather inline all that without any MAX_EA_NAME_LEN?

I think at least GCC and CLANG should be smart enough to optimise the
strlen() on a static string.

Regards,
Philippe

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Corinna Vinschen]

On Jun  6 17:17, Philippe Cerfon wrote:
> On Tue, Jun 6, 2023 at 3:28 PM Corinna Vinschen
> <corinna-cygwin@cygwin.com> wrote:
> > Yes, it is wrong, because the value of MAX_EA_NAME_LEN / XATTR_NAME_MAX
> > is used for array size definitions as well as comparisons.
> 
> Do you prefer to keep MAX_EA_NAME_LEN and just have that set like,
> perhaps with an additional comment that explains it:
>    #define MAX_EA_NAME_LEN    (XATTR_NAME_MAX + 1 - strlen("user."))
> or rather inline all that without any MAX_EA_NAME_LEN?

Hmm, the comparisons would have to check for XATTR_NAME_MAX anyway,
so maybe inlining is cleaner in this case.


Greets,
Corinna

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Philippe Cerfon]

[-- Attachment #1: Type: text/plain, Size: 302 bytes --]

Hey Corinna.

On Wed, Jun 7, 2023 at 12:06 PM Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> Hmm, the comparisons would have to check for XATTR_NAME_MAX anyway,
> so maybe inlining is cleaner in this case.

Please have a look at the updated and attached patches.

Thanks,
Philippe.

[-- Attachment #2: 0001-Cygwin-export-XATTR_-NAME-SIZE-LIST-_MAX.patch --]
[-- Type: text/x-patch, Size: 1099 bytes --]

From 82e2ff6f52d7401210247ae396ce3f1f115d93f0 Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 30 May 2023 13:16:18 +0200
Subject: [PATCH 1/2] Cygwin: export XATTR_{NAME,SIZE,LIST}_MAX

These are used for example by CPython.

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/include/cygwin/limits.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/winsup/cygwin/include/cygwin/limits.h b/winsup/cygwin/include/cygwin/limits.h
index aefc7c7bd..ea3e2836a 100644
--- a/winsup/cygwin/include/cygwin/limits.h
+++ b/winsup/cygwin/include/cygwin/limits.h
@@ -56,4 +56,11 @@ details. */
 #define __PATH_MAX 4096
 #define __PIPE_BUF 4096
 
+/* XATTR_NAME_MAX is the maximum XATTR name length excluding the null
+ * terminator. Since only XATTRs in the `user' namespace are allowed and the
+ * `user.' prefix is not stored, the maximum is increased by 5. */
+#define XATTR_NAME_MAX 260
+#define XATTR_SIZE_MAX 65536
+#define XATTR_LIST_MAX 65536
+
 #endif /* _CYGWIN_LIMITS_H__ */
-- 
2.40.1


[-- Attachment #3: 0002-Cygwin-use-new-XATTR_-NAME-SIZE-_MAX-instead-of-MAX_.patch --]
[-- Type: text/x-patch, Size: 3150 bytes --]

From 34a2e210082ddbf87d2c9569b6b14775a6cc5b95 Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 6 Jun 2023 02:52:49 +0200
Subject: [PATCH 2/2] Cygwin: use new XATTR_{NAME,SIZE}_MAX instead of
 MAX_EA_{NAME,VALUE}_LEN

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
---
 winsup/cygwin/ntea.cc | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/winsup/cygwin/ntea.cc b/winsup/cygwin/ntea.cc
index a400fcb2b..ef1fdf4cc 100644
--- a/winsup/cygwin/ntea.cc
+++ b/winsup/cygwin/ntea.cc
@@ -17,9 +17,7 @@ details. */
 #include "tls_pbuf.h"
 #include <stdlib.h>
 #include <attr/xattr.h>
-
-#define MAX_EA_NAME_LEN    256
-#define MAX_EA_VALUE_LEN 65536
+#include <cygwin/limits.h>
 
 /* At least one maximum sized entry fits.
    CV 2014-04-04: NtQueryEaFile function chokes on buffers bigger than 64K
@@ -27,13 +25,14 @@ details. */
 		  on a remote share, at least on Windows 7 and later.
 		  In theory the buffer should have a size of
 
-		    sizeof (FILE_FULL_EA_INFORMATION) + MAX_EA_NAME_LEN
-		    + MAX_EA_VALUE_LEN
+		    sizeof (FILE_FULL_EA_INFORMATION)
+		    + (XATTR_NAME_MAX + 1 - strlen("user."))
+		    + XATTR_SIZE_MAX
 
 		  (65804 bytes), but we're opting for simplicity here, and
 		  a 64K buffer has the advantage that we can use a tmp_pathbuf
 		  buffer, rather than having to alloca 64K from stack. */
-#define EA_BUFSIZ MAX_EA_VALUE_LEN
+#define EA_BUFSIZ XATTR_SIZE_MAX
 
 #define NEXT_FEA(p) ((PFILE_FULL_EA_INFORMATION) (p->NextEntryOffset \
 		     ? (char *) p + p->NextEntryOffset : NULL))
@@ -55,7 +54,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
      returns the last EA entry of the file infinitely.  Even utilizing the
      optional EaIndex only helps marginally.  If you use that, the last
      EA in the file is returned twice. */
-  char lastname[MAX_EA_NAME_LEN];
+  char lastname[(XATTR_NAME_MAX + 1 - strlen("user."))];
 
   __try
     {
@@ -95,7 +94,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
 	      __leave;
 	    }
 
-	  if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
+	  if ((nlen = strlen (name)) >= (XATTR_NAME_MAX + 1 - strlen("user.")))
 	    {
 	      set_errno (EINVAL);
 	      __leave;
@@ -197,7 +196,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
 		  /* For compatibility with Linux, we always prepend "user." to
 		     the attribute name, so effectively we only support user
 		     attributes from a application point of view. */
-		  char tmpbuf[MAX_EA_NAME_LEN * 2];
+		  char tmpbuf[(XATTR_NAME_MAX + 1 - strlen("user.")) * 2];
 		  char *tp = stpcpy (tmpbuf, "user.");
 		  stpcpy (tp, fea->EaName);
 		  /* NTFS stores all EA names in uppercase unfortunately.  To
@@ -297,7 +296,7 @@ write_ea (HANDLE hdl, path_conv &pc, const char *name, const char *value,
       /* Skip "user." prefix. */
       name += 5;
 
-      if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
+      if ((nlen = strlen (name)) >= (XATTR_NAME_MAX + 1 - strlen("user.")))
 	{
 	  set_errno (EINVAL);
 	  __leave;
-- 
2.40.1

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Corinna Vinschen]

Hi Philippe,

On Jun 16 16:09, Philippe Cerfon wrote:
> Hey Corinna.
> 
> On Wed, Jun 7, 2023 at 12:06 PM Corinna Vinschen
> <corinna-cygwin@cygwin.com> wrote:
> > Hmm, the comparisons would have to check for XATTR_NAME_MAX anyway,
> > so maybe inlining is cleaner in this case.
> 
> Please have a look at the updated and attached patches.
> 
> Thanks,
> Philippe.

Oh well. Now that I see it in real life, my idea to use the entire
expression inline wasn't that great, it seems... 

I didn't want to keep MAX_EA_NAME_LEN because now that we have an
official name for the value, having an unofficial name using a different
naming convention is a bit weird.

On the other hand, having a macro for the expression certainly looks
much cleaner.  Also, only one place to change (should a change ever be
necessary).

Sorry about that.

What do you think about something like _XATTR_NAME_MAX_ONDISK_?

I can also just push the patches and we discuss this further afterwards,
your call.


Thanks,
Corinna



> @@ -55,7 +54,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
>       returns the last EA entry of the file infinitely.  Even utilizing the
>       optional EaIndex only helps marginally.  If you use that, the last
>       EA in the file is returned twice. */
> -  char lastname[MAX_EA_NAME_LEN];
> +  char lastname[(XATTR_NAME_MAX + 1 - strlen("user."))];
>  
>    __try
>      {
> @@ -95,7 +94,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
>  	      __leave;
>  	    }
>  
> -	  if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
> +	  if ((nlen = strlen (name)) >= (XATTR_NAME_MAX + 1 - strlen("user.")))
>  	    {
>  	      set_errno (EINVAL);
>  	      __leave;
> @@ -197,7 +196,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
>  		  /* For compatibility with Linux, we always prepend "user." to
>  		     the attribute name, so effectively we only support user
>  		     attributes from a application point of view. */
> -		  char tmpbuf[MAX_EA_NAME_LEN * 2];
> +		  char tmpbuf[(XATTR_NAME_MAX + 1 - strlen("user.")) * 2];
>  		  char *tp = stpcpy (tmpbuf, "user.");
>  		  stpcpy (tp, fea->EaName);
>  		  /* NTFS stores all EA names in uppercase unfortunately.  To
> @@ -297,7 +296,7 @@ write_ea (HANDLE hdl, path_conv &pc, const char *name, const char *value,
>        /* Skip "user." prefix. */
>        name += 5;
>  
> -      if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
> +      if ((nlen = strlen (name)) >= (XATTR_NAME_MAX + 1 - strlen("user.")))
>  	{
>  	  set_errno (EINVAL);
>  	  __leave;
> -- 
> 2.40.1
> 

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Philippe Cerfon]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

Hey.

On Fri, Jun 16, 2023 at 5:04 PM Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> Oh well. Now that I see it in real life, my idea to use the entire
> expression inline wasn't that great, it seems...

^^


> I didn't want to keep MAX_EA_NAME_LEN because now that we have an
> official name for the value, having an unofficial name using a different
> naming convention is a bit weird.
>
> On the other hand, having a macro for the expression certainly looks
> much cleaner.  Also, only one place to change (should a change ever be
> necessary).

Does both make sense.


> Sorry about that.

No worries :-)


> What do you think about something like _XATTR_NAME_MAX_ONDISK_?

Really with trailing/leading underscores? If you try to keep it out of
the "official namespace", then I'd would perhaps make more sense to
mark this as being cygwin specific like CYGWIN_XATTR_NAME_MAX_ONDISK
or so?
Also - may be nitpicking - but storage is not really guaranteed to be
a disk anymore. Maybe ONSTORAGE instead? But admittedly ONDISK sounds
more common ("on disk format", etc.).

> I can also just push the patches and we discuss this further afterwards,
> your call.

Well you know the naming convention used in your code much better than I do.

Attached patches use _XATTR_NAME_MAX_ONDISK_ as you proposed.

Just pick whichever name you like best, and either tell me and I
provide a new patch, or just sed 's/_XATTR_NAME_MAX_ONDISK_/foobar/g'
(+ maybe align text wrapping of comments if necessary).


Thanks,
Philippe

[-- Attachment #2: 0001-Cygwin-export-XATTR_-NAME-SIZE-LIST-_MAX.patch --]
[-- Type: text/x-patch, Size: 1099 bytes --]

From 82e2ff6f52d7401210247ae396ce3f1f115d93f0 Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 30 May 2023 13:16:18 +0200
Subject: [PATCH 1/2] Cygwin: export XATTR_{NAME,SIZE,LIST}_MAX

These are used for example by CPython.

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
 winsup/cygwin/include/cygwin/limits.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/winsup/cygwin/include/cygwin/limits.h b/winsup/cygwin/include/cygwin/limits.h
index aefc7c7bd..ea3e2836a 100644
--- a/winsup/cygwin/include/cygwin/limits.h
+++ b/winsup/cygwin/include/cygwin/limits.h
@@ -56,4 +56,11 @@ details. */
 #define __PATH_MAX 4096
 #define __PIPE_BUF 4096
 
+/* XATTR_NAME_MAX is the maximum XATTR name length excluding the null
+ * terminator. Since only XATTRs in the `user' namespace are allowed and the
+ * `user.' prefix is not stored, the maximum is increased by 5. */
+#define XATTR_NAME_MAX 260
+#define XATTR_SIZE_MAX 65536
+#define XATTR_LIST_MAX 65536
+
 #endif /* _CYGWIN_LIMITS_H__ */
-- 
2.40.1


[-- Attachment #3: 0002-Cygwin-use-new-XATTR_-NAME-SIZE-_MAX-instead-of-MAX_.patch --]
[-- Type: text/x-patch, Size: 3239 bytes --]

From 4c94f80fc817bee0e59da39397391cf2409cf029 Mon Sep 17 00:00:00 2001
From: Philippe Cerfon <philcerf@gmail.com>
Date: Tue, 6 Jun 2023 02:52:49 +0200
Subject: [PATCH 2/2] Cygwin: use new XATTR_{NAME,SIZE}_MAX instead of
 MAX_EA_{NAME,VALUE}_LEN

Signed-off-by: Philippe Cerfon <philcerf@gmail.com>
---
 winsup/cygwin/ntea.cc | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/winsup/cygwin/ntea.cc b/winsup/cygwin/ntea.cc
index a400fcb2b..70815649c 100644
--- a/winsup/cygwin/ntea.cc
+++ b/winsup/cygwin/ntea.cc
@@ -17,9 +17,11 @@ details. */
 #include "tls_pbuf.h"
 #include <stdlib.h>
 #include <attr/xattr.h>
+#include <cygwin/limits.h>
 
-#define MAX_EA_NAME_LEN    256
-#define MAX_EA_VALUE_LEN 65536
+/* On storage the `user.` prefix is not included but the terminating null byte
+   is needed.*/
+#define _XATTR_NAME_MAX_ONDISK_ (XATTR_NAME_MAX - strlen("user.") + 1)
 
 /* At least one maximum sized entry fits.
    CV 2014-04-04: NtQueryEaFile function chokes on buffers bigger than 64K
@@ -27,13 +29,13 @@ details. */
 		  on a remote share, at least on Windows 7 and later.
 		  In theory the buffer should have a size of
 
-		    sizeof (FILE_FULL_EA_INFORMATION) + MAX_EA_NAME_LEN
-		    + MAX_EA_VALUE_LEN
+		    sizeof (FILE_FULL_EA_INFORMATION) + _XATTR_NAME_MAX_ONDISK_
+		    + XATTR_SIZE_MAX
 
 		  (65804 bytes), but we're opting for simplicity here, and
 		  a 64K buffer has the advantage that we can use a tmp_pathbuf
 		  buffer, rather than having to alloca 64K from stack. */
-#define EA_BUFSIZ MAX_EA_VALUE_LEN
+#define EA_BUFSIZ XATTR_SIZE_MAX
 
 #define NEXT_FEA(p) ((PFILE_FULL_EA_INFORMATION) (p->NextEntryOffset \
 		     ? (char *) p + p->NextEntryOffset : NULL))
@@ -55,7 +57,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
      returns the last EA entry of the file infinitely.  Even utilizing the
      optional EaIndex only helps marginally.  If you use that, the last
      EA in the file is returned twice. */
-  char lastname[MAX_EA_NAME_LEN];
+  char lastname[_XATTR_NAME_MAX_ONDISK_];
 
   __try
     {
@@ -95,7 +97,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
 	      __leave;
 	    }
 
-	  if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
+	  if ((nlen = strlen (name)) >= _XATTR_NAME_MAX_ONDISK_)
 	    {
 	      set_errno (EINVAL);
 	      __leave;
@@ -197,7 +199,7 @@ read_ea (HANDLE hdl, path_conv &pc, const char *name, char *value, size_t size)
 		  /* For compatibility with Linux, we always prepend "user." to
 		     the attribute name, so effectively we only support user
 		     attributes from a application point of view. */
-		  char tmpbuf[MAX_EA_NAME_LEN * 2];
+		  char tmpbuf[_XATTR_NAME_MAX_ONDISK_ * 2];
 		  char *tp = stpcpy (tmpbuf, "user.");
 		  stpcpy (tp, fea->EaName);
 		  /* NTFS stores all EA names in uppercase unfortunately.  To
@@ -297,7 +299,7 @@ write_ea (HANDLE hdl, path_conv &pc, const char *name, const char *value,
       /* Skip "user." prefix. */
       name += 5;
 
-      if ((nlen = strlen (name)) >= MAX_EA_NAME_LEN)
+      if ((nlen = strlen (name)) >= _XATTR_NAME_MAX_ONDISK_)
 	{
 	  set_errno (EINVAL);
 	  __leave;
-- 
2.40.1

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Corinna Vinschen]

On Jun 16 17:43, Philippe Cerfon wrote:
> Hey.
> 
> On Fri, Jun 16, 2023 at 5:04 PM Corinna Vinschen
> <corinna-cygwin@cygwin.com> wrote:
> > Oh well. Now that I see it in real life, my idea to use the entire
> > expression inline wasn't that great, it seems...
> 
> ^^
> 
> 
> > I didn't want to keep MAX_EA_NAME_LEN because now that we have an
> > official name for the value, having an unofficial name using a different
> > naming convention is a bit weird.
> >
> > On the other hand, having a macro for the expression certainly looks
> > much cleaner.  Also, only one place to change (should a change ever be
> > necessary).
> 
> Does both make sense.
> 
> 
> > Sorry about that.
> 
> No worries :-)
> 
> 
> > What do you think about something like _XATTR_NAME_MAX_ONDISK_?
> 
> Really with trailing/leading underscores? If you try to keep it out of
> the "official namespace", then I'd would perhaps make more sense to
> mark this as being cygwin specific like CYGWIN_XATTR_NAME_MAX_ONDISK
> or so?
> Also - may be nitpicking - but storage is not really guaranteed to be
> a disk anymore. Maybe ONSTORAGE instead? But admittedly ONDISK sounds
> more common ("on disk format", etc.).

You're right, of course.  Disk is just like everyone talks about it.
Even a SSD has "disk" in it's name :)

> > I can also just push the patches and we discuss this further afterwards,
> > your call.
> 
> Well you know the naming convention used in your code much better than I do.
> 
> Attached patches use _XATTR_NAME_MAX_ONDISK_ as you proposed.
> 
> Just pick whichever name you like best, and either tell me and I
> provide a new patch, or just sed 's/_XATTR_NAME_MAX_ONDISK_/foobar/g'
> (+ maybe align text wrapping of comments if necessary).

Let's keep it at that.  I pushed your patchset.


Thanks!
Corinna

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Philippe Cerfon]

On Fri, Jun 16, 2023 at 9:49 PM Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> Even a SSD has "disk" in it's name :)

That actually stands for drive :-)

> Let's keep it at that.  I pushed your patchset.

Thanks for merging! :-)

Any rough estimate when this will be in a live release?

Regards and best wishes,
Philippe

Re: [PATCH] include/cygwin/limits.h: add XATTR_{NAME,SIZE,LIST}_MAX

[Corinna Vinschen]

On Jun 16 21:52, Philippe Cerfon wrote:
> On Fri, Jun 16, 2023 at 9:49 PM Corinna Vinschen
> <corinna-cygwin@cygwin.com> wrote:
> > Even a SSD has "disk" in it's name :)
> 
> That actually stands for drive :-)

Huh, ok.  To me it always was "solid state disk", but it's certainly
not the first time I'm wrong :)))

> > Let's keep it at that.  I pushed your patchset.
> 
> Thanks for merging! :-)
> 
> Any rough estimate when this will be in a live release?

End of this year. That's the plan for 3.5.0.


Greets,
Corinna