Re: [PR] Implement apr_shm_perms_set for POSIX shm_o pen/"classic mmap" shmem [apr]

rpluem commented on code in PR #59:
URL: https://github.com/apache/apr/pull/59#discussion_r1706434199


##########
shmem/unix/shm.c:
##########
@@ -705,6 +705,47 @@ APR_PERMS_SET_IMPLEMENT(shm)
     if (shmctl(shmid, IPC_SET, &shmbuf) == -1) {
         return errno;
     }
+    return APR_SUCCESS;
+#elif APR_USE_SHMEM_MMAP_SHM
+    apr_shm_t *shm = (apr_shm_t *)theshm;
+    const char *shm_name;
+    int fd;
+    apr_status_t rv;
+
+    if (!shm->filename)
+        return APR_ENOTIMPL;
+
+    shm_name = make_shm_open_safe_name(shm->filename, shm->pool);
+
+    fd = shm_open(shm_name, O_RDWR, 0);
+    if (fd == -1)
+        return errno;
+
+    if (fchown(fd, uid, gid)) {
+        rv = errno;
+        close(fd);
+        return rv;
+    }
+
+    if (fchmod(fd, apr_unix_perms2mode(perms))) {
+        rv = errno;
+        close(fd);
+        return rv;
+    }
+

Review Comment:
   I guess we miss to close the fd here:
   
   ```suggestion
       close(fd);
   ```



##########
shmem/unix/shm.c:
##########
@@ -705,6 +705,47 @@ APR_PERMS_SET_IMPLEMENT(shm)
     if (shmctl(shmid, IPC_SET, &shmbuf) == -1) {
         return errno;
     }
+    return APR_SUCCESS;
+#elif APR_USE_SHMEM_MMAP_SHM
+    apr_shm_t *shm = (apr_shm_t *)theshm;
+    const char *shm_name;
+    int fd;
+    apr_status_t rv;
+
+    if (!shm->filename)
+        return APR_ENOTIMPL;
+
+    shm_name = make_shm_open_safe_name(shm->filename, shm->pool);
+
+    fd = shm_open(shm_name, O_RDWR, 0);
+    if (fd == -1)
+        return errno;
+
+    if (fchown(fd, uid, gid)) {
+        rv = errno;
+        close(fd);
+        return rv;
+    }
+
+    if (fchmod(fd, apr_unix_perms2mode(perms))) {
+        rv = errno;
+        close(fd);
+        return rv;
+    }
+
+    return APR_SUCCESS;
+#elif APR_USE_SHMEM_MMAP_TMP
+    apr_shm_t *shm = (apr_shm_t *)theshm;
+
+    if (!shm->filename)
+        return APR_ENOTIMPL;
+
+    if (chown(shm->filename, uid, gid))
+        return errno;
+
+    if (chmod(shm->filename, apr_unix_perms2mode(perms)))
+        return errno;
+
     return APR_SUCCESS;

Review Comment:
   Do we need to use the fchown / fchmod approach here as well to prevent races between chown and chmod or is this just overkill?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]