You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/06/13 12:02:25 UTC
[GitHub] [incubator-nuttx] pussuw opened a new pull request, #6423: sched/env: Fix the return value of unsetenv()
pussuw opened a new pull request, #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423
If the environment variable does not exist, the function succeeds, as
defined by POSIX.
## Summary
Fixes return value of unsetenv, value was accidentally broken in #6083
## Impact
## Testing
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895811358
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
```suggestion
DEBUGASSERT(name && group);
/* Check the incoming parameter */
if (*name == '\0' || strchr (name, '=') != NULL)
```
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
+ {
+ set_errno(EINVAL);
+ return -EINVAL;
Review Comment:
```suggestion
return ERROR;
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895661902
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
Yes we could add check for the incoming parameter too. The man page is a bit ambiguous in my opinion about this.
For `setenv()`it makes perfect sense to check for the incoming 'name' parameter.
For `unsetenv()` it is ambiguous. The check is for 'name' parameter validity, but no such env variable can exist, as setenv() checks for parameter validity. As such name does not exist, should the function return success ?
EDIT: There seem to be contradicting man pages on this also, but no page specifically **prohibits** testing for the incoming parameter 'name' for validity and returning EINVAL for bad parameter is allowed by all the versions I could find, so it is safer/better to do the check!
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895821153
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
My thought was not to crash the whole system here but I can revert to the old implementation too.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r896409087
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
At least setenv() uses the original style `*name == '\0'`also so I'll keep the original formatting
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
Thanks, I missed this
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895661902
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
Yes we could add check for the incoming parameter too. The man page is a bit ambiguous in my opinion about this.
For `setenv()`it makes perfect sense to check for the incoming 'name' parameter.
For `unsetenv()` it is ambiguous. The check is for 'name' parameter validity, but no such env variable can exist, as setenv() checks for parameter validity. As such name does not exist, should the function return success ?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895827664
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
I'm not sure. We can leave `NULL` check as `EINVAL`. That makes sense. I just was thinking that POSIX states nothing about NULL case, but only covers empty string and `'='` holder string. I'm fine to keep the `NULL` check as recoverable error. Not sure how Linux manages that.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895677603
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
Yup, there has clearly been some iteration with that since I can find a contradicting (at least ambiguous!) man page (first result in google): https://man7.org/linux/man-pages/man3/setenv.3.html
But like said, neither **prohibit** returning EINVAL if the incoming parameter "name" is not valid so it is better to do the test.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895860595
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
Just made a quick test with Linux and got the next compilation warning:
```
test.c: In function ‘main’:
test.c:13:11: warning: null argument where non-null required (argument 1) [-Wnonnull]
13 | int ret = unsetenv(NULL);
```
and execution result:
```
unsetenv(NULL) ret -1, errno 22
```
Update:
Changing `NULL` to pointer eliminates the warning.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#issuecomment-1153842652
Missed that env_removevar() takes the index in. Fixed now!
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895673222
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/unsetenv.html says
```
The unsetenv() function shall remove an environment variable from the environment of the calling process.
The name argument points to a string, which is the name of the variable to be removed. The named argument
shall not contain an '=' character. If the named variable does not exist in the current environment, the environment
shall be unchanged and the function is considered to have completed successfully.
The unsetenv() function shall update the list of pointers to which environ points.
The unsetenv() function need not be thread-safe.
```
I do not see any contradiction. The `=` part is explicitly stated as `The named argument shall not contain an '=' character.` and to check this we need to check for an empty string as well :)
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895681153
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
Added the parameter check as separate commit
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895661902
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
Yes we could add check for the incoming parameter too. The man page is a bit ambiguous in my opinion about this.
For `setenv()`it makes perfect sense to check for the incoming 'name' parameter.
For `unsetenv()` it is ambiguous. The check is for 'name' parameter validity, but no such env variable can exist, as setenv() checks for parameter validity. As such name does not exist, should the function return success ?
EDIT: There seem to be contradicting man pages on this also, but no page specifically **prohibits** testing for the incoming parameter 'name' for validity and returning EINVAL for a bad parameter is allowed by all the versions I could find, so it is safer/better to do the check!
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895661902
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
Yes we could add check for the incoming parameter too. The man page is a bit ambiguous in my opinion about this.
For `setenv()`it makes perfect sense to check for the incoming 'name' parameter.
For `unsetenv()` it is ambiguous. The check is for 'name' parameter validity, but no such env variable can exist, as setenv() checks for parameter validity. As such name does not exist, should the function return success ?
EDIT: There seem to be contradicting man pages on this also, but no page specifically prohibits testing for the incoming parameter 'name' for validity so it is safer/better to do the check!
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895673222
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/unsetenv.html says
```
The unsetenv() function shall remove an environment variable from the environment of the calling process.
The name argument points to a string, which is the name of the variable to be removed. The named argument
shall not contain an '=' character. If the named variable does not exist in the current environment, the
environment shall be unchanged and the function is considered to have completed successfully.
The unsetenv() function shall update the list of pointers to which environ points.
The unsetenv() function need not be thread-safe.
```
I do not see any contradiction. The `=` part is explicitly stated as `The named argument shall not contain an '=' character.` and to check this we need to check for an empty string as well :)
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895661902
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
Yes we could add check for the incoming parameter too. The man page is a bit ambiguous in my opinion about this.
For `setenv()`it makes perfect sense to check for the incoming parameter.
For `unsetenv()` it is ambiguous. The check is for parameter validity, but no such env variable can exist, as setenv() checks for parameter validity. As such name does not exist, should the function return success ?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895653622
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
According to POSIX `unsetenv` can fail with
```
[EINVAL]
The name argument points to an empty string, or points to a string containing an '=' character.
```
Leaving here just for a note. Should be fixed by a separate PR.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895991361
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
```suggestion
if (name == NULL || *name == '\0' || strchr(name, '=') != NULL)
```
I believe that white space should not exist.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895932656
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
Yes glibc has test for NULL
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895932656
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
Yes glibc tests for the NULL pointer
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895673222
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,22 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
DEBUGASSERT(name && group);
/* Check if the variable exists */
sched_lock();
- if (group && (ret = env_findvar(group, name)) >= 0)
+ if (group && (idx = env_findvar(group, name)) >= 0)
{
/* It does! Remove the name=value pair from the environment. */
- env_removevar(group, ret);
- ret = OK;
+ env_removevar(group, idx);
}
sched_unlock();
- return ret;
+ return OK;
Review Comment:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/unsetenv.html says
```
The unsetenv() function shall remove an environment variable from the environment of the calling process. The name argument points to a string, which is the name of the variable to be removed. The named argument shall not contain an '=' character. If the named variable does not exist in the current environment, the environment shall be unchanged and the function is considered to have completed successfully.
The unsetenv() function shall update the list of pointers to which environ points.
The unsetenv() function need not be thread-safe.
```
I do not see any contradiction. The `=` part is explicitly stated as `The named argument shall not contain an '=' character.` and to check this we need to check for an empty string as well :)
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895818558
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
+ {
+ set_errno(EINVAL);
+ return -EINVAL;
Review Comment:
Yes should return -1, will fix
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895860595
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
Just made a quick test with Linux and got the next compilation warning:
```
test.c: In function ‘main’:
test.c:13:11: warning: null argument where non-null required (argument 1) [-Wnonnull]
13 | int ret = unsetenv(NULL);
```
and execution result:
```
unsetenv(NULL) ret -1, errno 22
```
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a diff in pull request #6423: sched/env: Fix the return value of unsetenv()
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on code in PR #6423:
URL: https://github.com/apache/incubator-nuttx/pull/6423#discussion_r895993180
##########
sched/environ/env_unsetenv.c:
##########
@@ -61,23 +61,30 @@ int unsetenv(FAR const char *name)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct task_group_s *group = rtcb->group;
- int ret = OK;
+ int idx;
- DEBUGASSERT(name && group);
+ DEBUGASSERT(group);
+
+ /* Check the incoming parameter */
+
+ if (name == NULL || *name == '\0' || strchr (name, '=') != NULL)
Review Comment:
```suggestion
if (name == NULL || name[0] == '\0' || strchr (name, '=') != NULL)
```
nit: just a suggestion, at least for me this way becomes more easily readable.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org