Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Do not merge]fs/mqueue/mq_check.c: group level error check for mq operations #6570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion os/fs/mqueue/Make.defs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@

ifneq ($(CONFIG_DISABLE_MQUEUE),y)

CSRCS += mq_open.c mq_close.c mq_unlink.c
CSRCS += mq_open.c mq_close.c mq_unlink.c mq_check.c

# Include POSIX message queue build support

Expand Down
108 changes: 108 additions & 0 deletions os/fs/mqueue/mq_check.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/****************************************************************************
*
* Copyright 2024 Samsung Electronics All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND,
* either express or implied. See the License for the specific
* language governing permissions and limitations under the License.
*
****************************************************************************/
/************************************************************************
* fs/mqueue/mq_check.c
*
* Copyright (C) 2007, 2009, 2014 Gregory Nutt. All rights reserved.
* Author: Gregory Nutt <[email protected]>
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
*
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in
* the documentation and/or other materials provided with the
* distribution.
* 3. Neither the name NuttX nor the names of its contributors may be
* used to endorse or promote products derived from this software
* without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
* FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
* COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
* INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
* BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS
* OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
* AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
* ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*
************************************************************************/

/************************************************************************
* Included Files
************************************************************************/

#include <tinyara/config.h>

#include <stdio.h>
#include <mqueue.h>
#include <sched.h>
#include <assert.h>
#include <errno.h>
#include <queue.h>

#include <tinyara/mqueue.h>
#include <tinyara/sched.h>

/************************************************************************
* Public Functions
************************************************************************/

/************************************************************************
* Name: mq_check
*
* Description:
* This function checks if a message queue descriptor is present in the
* calling task's group list of mq des.
*
* Parameters:
* mqdes - Message queue descriptor
*
* Return Value:
* OK - if mqdes is present in the calling task group list of mqdes
* ERROR - if mqdes is not present in the calling task group of mqdes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not return ERROR, it does EBADF.

*
************************************************************************/

int mq_check(mqd_t mqdes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename the function to show "check what".

{
int ret = -EBADF;
mqd_t mqdes_ptr;

FAR struct task_group_s *group = sched_self()->group;
Copy link
Contributor

@jeongarmy jeongarmy Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to check mqdes and group because most of mq functions except mq_close don't check it.
DEBUGASSERT(mqdes != NULL && group != NULL);


sched_lock();
mqdes_ptr = (mqd_t)sq_peek(&group->tg_msgdesq);
while (mqdes_ptr) {
if (mqdes_ptr == mqdes) {
ret = OK;
break;
}
mqdes_ptr = (mqd_t)sq_next(mqdes_ptr);
}
sched_unlock();

return ret;
}
12 changes: 1 addition & 11 deletions os/fs/mqueue/mq_close.c
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ int mq_close_group(mqd_t mqdes, FAR struct task_group_s *group)
int ret = OK;
FAR struct mqueue_inode_s *msgq;
FAR struct inode *inode;
mqd_t mqdes_ptr;

DEBUGASSERT(mqdes != NULL && group != NULL);

Expand All @@ -105,17 +104,8 @@ int mq_close_group(mqd_t mqdes, FAR struct task_group_s *group)
if (mqdes) {
sched_lock();

/* Check that mqdes is in one's group */
mqdes_ptr = (mqd_t)sq_peek(&group->tg_msgdesq);
while (mqdes_ptr) {
if (mqdes_ptr == mqdes) {
break;
}
mqdes_ptr = (mqd_t)sq_next(mqdes_ptr);
}

/* If there is no mqdes in one's group, skip to desclose and inode release. */
if (mqdes_ptr != NULL) {
if (mq_check(mqdes) == OK) {

/* Find the message queue associated with the message descriptor */

Expand Down
7 changes: 7 additions & 0 deletions os/include/mqueue.h
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,13 @@ int mq_setattr(mqd_t mqdes, FAR const struct mq_attr *mq_stat, FAR struct mq_att
* @since TizenRT v1.0
*/
int mq_getattr(mqd_t mqdes, FAR struct mq_attr *mq_stat);
/**
* @brief get message queue attributes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

description looks wrong.

* @details @b #include <mqueue.h> \n
* POSIX API (refer to : http://pubs.opengroup.org/onlinepubs/9699919799/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the POSIX API?

* @since TizenRT v1.0
*/
int mq_check(mqd_t mqdes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an internal api for mqueue, we need not expose it to outsize. Please check if we can we move this to some internal header file.


#undef EXTERN
#ifdef __cplusplus
Expand Down
4 changes: 4 additions & 0 deletions os/kernel/mqueue/mq_getattr.c
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ int mq_getattr(mqd_t mqdes, struct mq_attr *mq_stat)
{
int ret = ERROR;

if (mq_check(mqdes) != OK) {
return ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to set the errno before we return.

}

if (mqdes && mq_stat) {
/* Return the attributes */

Expand Down
5 changes: 5 additions & 0 deletions os/kernel/mqueue/mq_notify.c
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,11 @@ int mq_notify(mqd_t mqdes, const struct sigevent *notification)
return ERROR;
}

if (mq_check(mqdes) != OK) {
set_errno(EBADF);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should use the return value instead of hard coding here.

return ERROR;
}

/* Get a pointer to the message queue */

msgq = mqdes->msgq;
Expand Down
6 changes: 6 additions & 0 deletions os/kernel/mqueue/mq_receive.c
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ ssize_t mq_receive(mqd_t mqdes, FAR char *msg, size_t msglen, FAR int *prio)
/* mq_receive() is a cancellation point */
(void)enter_cancellation_point();

if (mq_check(mqdes) != OK) {
leave_cancellation_point();
set_errno(EINVAL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it has EINVAL, when it has EBADF?

return ERROR;
}

if (mq_verifyreceive(mqdes, msg, msglen) != OK) {
leave_cancellation_point();
return ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before ERROR return, we should set proper errno.

 * Return Value:
 *   One success, the length of the selected message in bytes is returned.
 *   On failure, -1 (ERROR) is returned and the errno is set appropriately:
 *
 *   EAGAIN   The queue was empty, and the O_NONBLOCK flag was set
 *            for the message queue description referred to by 'mqdes'.
 *   EPERM    Message queue opened not opened for reading.
 *   EMSGSIZE 'msglen' was less than the maxmsgsize attribute of the
 *            message queue.
 *   EINTR    The call was interrupted by a signal handler.
 *   EINVAL   Invalid 'msg' or 'mqdes'

Expand Down
9 changes: 8 additions & 1 deletion os/kernel/mqueue/mq_send.c
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@
*
* EAGAIN The queue was empty, and the O_NONBLOCK flag was set for the
* message queue description referred to by mqdes.
* EINVAL Either msg or mqdes is NULL or the value of prio is invalid.
* EINVAL Either msg or mqdes is NULL or the value of prio is invalid,
* or mqdes is present in task group's mq list.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be,

Suggested change
* or mqdes is present in task group's mq list.
* or mqdes is not present in task group's mq list.

But here too, I think EBADF is better instead of EINVAL.

* EPERM Message queue opened not opened for writing.
* EMSGSIZE 'msglen' was greater than the maxmsgsize attribute of the
* message queue.
Expand All @@ -149,6 +150,12 @@ int mq_send(mqd_t mqdes, FAR const char *msg, size_t msglen, int prio)
/* mq_send() is a cancellation point */
(void)enter_cancellation_point();

if (mq_check(mqdes) != OK) {
leave_cancellation_point();
set_errno(EINVAL);
return ERROR;
}

if (mq_verifysend(mqdes, msg, msglen, prio) != OK) {
leave_cancellation_point();
return ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous

Expand Down
4 changes: 4 additions & 0 deletions os/kernel/mqueue/mq_setattr.c
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ int mq_setattr(mqd_t mqdes, const struct mq_attr *mq_stat, struct mq_attr *oldst
{
int ret = ERROR;

if (mq_check(mqdes) != OK) {
return ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set errno is missnig

}

if (mqdes && mq_stat) {
/* Return the attributes if so requested */

Expand Down