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

["Zhao, Lihua (CN)" <Lihua.Zhao.CN@windriver.com>]

[-- 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);
}