[musl] [PATCH] mman: correct length check in __shm_mapname

[musl] [PATCH] mman: correct length check in __shm_mapname

[lihua.zhao.cn]

From: Lihua Zhao <lihua.zhao.cn@windriver.com>

changed the length check from `p-name > NAME_MAX` to
`p-name >= NAME_MAX` to correctly account for the null terminator.

Signed-off-by: Lihua Zhao <lihua.zhao.cn@windriver.com>
---
 src/mman/shm_open.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/mman/shm_open.c b/src/mman/shm_open.c
index 79784bd3..2359f067 100644
--- a/src/mman/shm_open.c
+++ b/src/mman/shm_open.c
@@ -15,7 +15,7 @@ char *__shm_mapname(const char *name, char *buf)
 		errno = EINVAL;
 		return 0;
 	}
-	if (p-name > NAME_MAX) {
+	if (p-name >= NAME_MAX) {
 		errno = ENAMETOOLONG;
 		return 0;
 	}
-- 
2.43.0

Re: [musl] [PATCH] mman: correct length check in __shm_mapname

[Rich Felker]

On Tue, Nov 05, 2024 at 09:06:33AM +0800, lihua.zhao.cn@windriver.com wrote:
> From: Lihua Zhao <lihua.zhao.cn@windriver.com>
> 
> changed the length check from `p-name > NAME_MAX` to
> `p-name >= NAME_MAX` to correctly account for the null terminator.
> 
> Signed-off-by: Lihua Zhao <lihua.zhao.cn@windriver.com>
> ---
>  src/mman/shm_open.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/mman/shm_open.c b/src/mman/shm_open.c
> index 79784bd3..2359f067 100644
> --- a/src/mman/shm_open.c
> +++ b/src/mman/shm_open.c
> @@ -15,7 +15,7 @@ char *__shm_mapname(const char *name, char *buf)
>  		errno = EINVAL;
>  		return 0;
>  	}
> -	if (p-name > NAME_MAX) {
> +	if (p-name >= NAME_MAX) {
>  		errno = ENAMETOOLONG;
>  		return 0;
>  	}
> -- 
> 2.43.0

This doesn't look correct. Can you explain what problem you think it's
solving?

Rich

RE: [musl] [PATCH] mman: correct length check in __shm_mapname

[Zhao, Lihua (CN)]

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

This issue is found by attached test case, it works well with glibc.

        sem_name[0] = '/';

        sem_name[NAME_MAX + 1] = '\0';

        memset(sem_name + 1, 'N', NAME_MAX);

        /* Create the semaphore */
        sem = sem_open(sem_name, O_CREAT, 0777, 1);

The above code will generate below string which has one '/' and 255 'N's:

"/NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN"

When call __shm_mapname, it firstly try to skip the first '/' character, name point to the first 'N' character, the p will point to the EOS, so the p-name equal 255, the original code won't enter the ENAMETOOLONG branch. The name string should end with EOS, and all valid characters should be less than or equal to 254.

Thanks,
Lihua

-----Original Message-----
From: Rich Felker <dalias@libc.org> 
Sent: Tuesday, November 5, 2024 9:47 AM
To: Zhao, Lihua (CN) <Lihua.Zhao.CN@windriver.com>
Cc: musl@lists.openwall.com
Subject: Re: [musl] [PATCH] mman: correct length check in __shm_mapname

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Tue, Nov 05, 2024 at 09:06:33AM +0800, lihua.zhao.cn@windriver.com wrote:
> From: Lihua Zhao <lihua.zhao.cn@windriver.com>
>
> changed the length check from `p-name > NAME_MAX` to `p-name >= 
> NAME_MAX` to correctly account for the null terminator.
>
> Signed-off-by: Lihua Zhao <lihua.zhao.cn@windriver.com>
> ---
>  src/mman/shm_open.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mman/shm_open.c b/src/mman/shm_open.c index 
> 79784bd3..2359f067 100644
> --- a/src/mman/shm_open.c
> +++ b/src/mman/shm_open.c
> @@ -15,7 +15,7 @@ char *__shm_mapname(const char *name, char *buf)
>               errno = EINVAL;
>               return 0;
>       }
> -     if (p-name > NAME_MAX) {
> +     if (p-name >= NAME_MAX) {
>               errno = ENAMETOOLONG;
>               return 0;
>       }
> --
> 2.43.0

This doesn't look correct. Can you explain what problem you think it's solving?

Rich

[-- Attachment #2: test_sem_open.c --]
[-- Type: text/plain, Size: 2366 bytes --]

#include <pthread.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#include <semaphore.h>
#include <errno.h>
#include <fcntl.h>


#ifndef VERBOSE
#define VERBOSE 1
#endif

#define FAILED(s)                           \
{                                   \
    printf("Test FAILED: %s\n", s);                 \
    exit(1);                         \
}

#define SEM_NAME  "/sem_unlink_5_1"

#ifdef NAME_MAX
#undef NAME_MAX
#endif

/******************************************************************************/
/***************************    Test case   ***********************************/
/******************************************************************************/

/* The main test function. */
int main(void)
{
    int ret, error;
    sem_t *sem;
    long NAME_MAX;
    char *sem_name;


    /* Get NAME_MAX value */
    NAME_MAX = pathconf("/", _PC_NAME_MAX);

#if VERBOSE > 0
    printf("NAME_MAX: %ld\n", NAME_MAX);
#endif

    if (NAME_MAX > 0) {
        /* create a semaphore with a name longer than NAME_MAX */
        sem_name = calloc(NAME_MAX + 2, sizeof(char));

        if (sem_name == NULL) {
            perror("Failed to allocate space for the semaphore name");
        }

        /* the space was allocated */
        sem_name[0] = '/';

        sem_name[NAME_MAX + 1] = '\0';

        memset(sem_name + 1, 'N', NAME_MAX);

        /* Create the semaphore */
        sem = sem_open(sem_name, O_CREAT, 0777, 1);

        if (sem != SEM_FAILED) {
            ret = sem_unlink(sem_name);
            error = errno;
            free(sem_name);

           if (ret == 0) {
                FAILED
                    ("The function did not return ENAMETOOLONG as expected");
            } else {
                printf("Error was %d: %s\n", error,
                       strerror(error));
                FAILED
                    ("Unable to unlink a semaphore which we just created");
            }
        }
#if VERBOSE > 0
        else {
            printf
                ("Creation of the semaphore failed with error %d: %s\n",
                 errno, strerror(errno));
        }

#endif

    }

    /* Test passed */
#if VERBOSE > 0
    printf("Test passed\n");

#endif
    exit(0);
}

Re: [musl] [PATCH] mman: correct length check in __shm_mapname

[Rich Felker]

On Tue, Nov 05, 2024 at 02:03:21AM +0000, Zhao, Lihua (CN) wrote:
> This issue is found by attached test case, it works well with glibc.
> 
>         sem_name[0] = '/';
> 
>         sem_name[NAME_MAX + 1] = '\0';
> 
>         memset(sem_name + 1, 'N', NAME_MAX);
> 
>         /* Create the semaphore */
>         sem = sem_open(sem_name, O_CREAT, 0777, 1);
> 
> The above code will generate below string which has one '/' and 255 'N's:
> 
> "/NNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNNN"
> 
> When call __shm_mapname, it firstly try to skip the first '/'
> character, name point to the first 'N' character, the p will point
> to the EOS, so the p-name equal 255, the original code won't enter
> the ENAMETOOLONG branch. The name string should end with EOS, and
> all valid characters should be less than or equal to 254.

This "should" is incorrect. A name consisting of 255 N's is valid not
an error. NAME_MAX is the maximum length of a file name (pathname
component) in bytes, not the amount of storage needed for such a
string buffer.

Reference:
https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/limits.h.html

"{NAME_MAX}
    Maximum number of bytes in a filename (not including the
    terminating null of a filename string)."

Rich

[musl] [PATCH v2] mman: correct length check in __shm_mapname

[lihua.zhao.cn]

From: Lihua Zhao <lihua.zhao.cn@windriver.com>

account for leading slashes when comparing against NAME_MAX.

Signed-off-by: Lihua Zhao <lihua.zhao.cn@windriver.com>
---
According to https://pubs.opengroup.org/onlinepubs/9799919799/:

leading <slash> character in name is implementation-defined, and that the length limits for the name argument are implementation-defined and need not be the same as the pathname limits {PATH_MAX} and {NAME_MAX}.

Although it is implementation-defined, glibc obviously calculates the lead slash.

 src/mman/shm_open.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/mman/shm_open.c b/src/mman/shm_open.c
index 79784bd3..07dbeb96 100644
--- a/src/mman/shm_open.c
+++ b/src/mman/shm_open.c
@@ -8,19 +8,19 @@

 char *__shm_mapname(const char *name, char *buf)
 {
-	char *p;
-	while (*name == '/') name++;
-	if (*(p = __strchrnul(name, '/')) || p==name ||
-	    (p-name <= 2 && name[0]=='.' && p[-1]=='.')) {
+	const char *s=name, *e;
+	while (*s == '/') s++;
+	if (*(e = __strchrnul(s, '/')) || e==s ||
+	    (e-s <= 2 && s[0]=='.' && e[-1]=='.')) {
 		errno = EINVAL;
 		return 0;
 	}
-	if (p-name > NAME_MAX) {
+	if (e-name > NAME_MAX) {
 		errno = ENAMETOOLONG;
 		return 0;
 	}
 	memcpy(buf, "/dev/shm/", 9);
-	memcpy(buf+9, name, p-name+1);
+	memcpy(buf+9, s, e-s+1);
 	return buf;
 }

--
2.34.1

Re: [musl] [PATCH v2] mman: correct length check in __shm_mapname

[Rich Felker]

On Tue, Nov 05, 2024 at 12:56:28PM +0800, lihua.zhao.cn@windriver.com wrote:
> From: Lihua Zhao <lihua.zhao.cn@windriver.com>
> 
> account for leading slashes when comparing against NAME_MAX.
> 
> Signed-off-by: Lihua Zhao <lihua.zhao.cn@windriver.com>
> ---

I'm still not clear what you're trying to achieve here. If the bug is
"it's different from glibc", that is not a bug.

> According to https://pubs.opengroup.org/onlinepubs/9799919799/:
> 
> leading <slash> character in name is implementation-defined, and
> that the length limits for the name argument are
> implementation-defined and need not be the same as the pathname
> limits {PATH_MAX} and {NAME_MAX}.
> 
> Although it is implementation-defined, glibc obviously calculates the lead slash.

Leading slash is not implementation-defined. The text you quoted says
the opposite if you didn't cut off the earlier part of the sentence:

"...except that the interpretation of <slash> characters other than
the"

A leading slash is necessary to portably open shared memory by a name
in a shared global namespace. Omitting it, or using slashes elsewhere
in the name, is what's implementation-defined.

Indeed the limits need not match NAME_MAX, but since we implement
named shared memory objects as filesystem objects, the implementation
choice we make is to have the limit match.

Rich

RE: [musl] [PATCH v2] mman: correct length check in __shm_mapname

[Zhao, Lihua (CN)]

Thank you for your support and respect your decision. Anyway, this is just a boundary management and is usually not used, so it is OK not to change it.

Lihua

-----Original Message-----
From: Rich Felker <dalias@libc.org> 
Sent: Tuesday, November 5, 2024 1:15 PM
To: Zhao, Lihua (CN) <Lihua.Zhao.CN@windriver.com>
Cc: musl@lists.openwall.com
Subject: Re: [musl] [PATCH v2] mman: correct length check in __shm_mapname

CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Tue, Nov 05, 2024 at 12:56:28PM +0800, lihua.zhao.cn@windriver.com wrote:
> From: Lihua Zhao <lihua.zhao.cn@windriver.com>
>
> account for leading slashes when comparing against NAME_MAX.
>
> Signed-off-by: Lihua Zhao <lihua.zhao.cn@windriver.com>
> ---

I'm still not clear what you're trying to achieve here. If the bug is "it's different from glibc", that is not a bug.

> According to https://pubs.opengroup.org/onlinepubs/9799919799/:
>
> leading <slash> character in name is implementation-defined, and that 
> the length limits for the name argument are implementation-defined and 
> need not be the same as the pathname limits {PATH_MAX} and {NAME_MAX}.
>
> Although it is implementation-defined, glibc obviously calculates the lead slash.

Leading slash is not implementation-defined. The text you quoted says the opposite if you didn't cut off the earlier part of the sentence:

"...except that the interpretation of <slash> characters other than the"

A leading slash is necessary to portably open shared memory by a name in a shared global namespace. Omitting it, or using slashes elsewhere in the name, is what's implementation-defined.

Indeed the limits need not match NAME_MAX, but since we implement named shared memory objects as filesystem objects, the implementation choice we make is to have the limit match.

Rich