Hi Hans,
So I guess it's ok to push V1 with comments from you and Anders.
Regards,
Minh
On 20/03/18 19:16, Hans Nordebäck wrote:
Hi Minh,
I think you can keep v1 but add the missing if stmt. The /proc/self/fd
directory should only contain open fd's and the current and parent directory.
Later you change stroixmax to the new string to integer utility, if needed,
(not likely it is needed though, I think).
/Regards HansN
-----Original Message-----
From: Minh Hon Chau [mailto:[email protected]]
Sent: den 20 mars 2018 08:29
To: Hans Nordebäck <[email protected]>; Anders Widell
<[email protected]>; [email protected]
Cc: [email protected]
Subject: Re: [PATCH 1/1] base: Only close inherited fd(s) after fork() in child
process [#2805]
Hi Hans,
I have just seen Anders created #2814 for the utility function, once
#2814 is done, we can change the integer conversion in this patch by new
utility function, maybe for a numbers of atoXX (atoi, atol,..) calls in many
other places for a better error handling. How do you think?
Thanks,
Minh
On 20/03/18 01:29, Hans Nordebäck wrote:
Hi Minh,
I think this additional check, current and parent, directory is enough
for V1 patch. The usage of the 2nd
parameter of strtol in V2 patch can be put in a utility function for a
broader use.
/Regards HansN
On 03/19/2018 08:50 AM, Minh Hon Chau wrote:
Hi Hans,
Agree that the check of "." and ".." should be added in V1.
This V2 I use the second parameter of strtol, it should ensure that
anything read from the fd directory is entirely digit, before close
the fd.
There should not be any alphabet-based directories other than ".",
".." and 0, 1, 2, ..etc, but the usage of 2nd parameter of strtol is
more generalized
So I think the check of strcmp is not needed?
Thanks,
Minh
On 19/03/18 18:00, Hans Nordebäck wrote:
Hi Minh,
my comment was that this check could be added:
if (strcmp(pentry->d_name, ".") == 0 || strcmp(pentry->d_name, "..")
== 0)
continue;
/Regards HansN
On 03/16/2018 01:27 PM, Minh Hon Chau wrote:
Hi Anders, Hans,
When I tested the patch, I did see the "." and ".." returned from
readdir, but the stroimax also return 0, so probably it won't be a
problem to close(0) more than once
But to be more safety, it should check the @second_ptr too, I will
update and send out a V2.
Thanks
Minh
On 16/03/18 23:00, Anders Widell wrote:
Hi!
See my comments below, marked AndersW2>.
regards,
Anders Widell
On 03/16/2018 12:39 PM, Hans Nordebäck wrote:
Hi Minh, ack with some comments, (on top of AndersW comments).
/Thanks HansN
On 03/15/2018 07:50 AM, Minh Chau wrote:
---
src/base/os_defs.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/base/os_defs.c b/src/base/os_defs.c index
6f9ec52..e914011 100644
--- a/src/base/os_defs.c
+++ b/src/base/os_defs.c
@@ -1052,14 +1052,29 @@ uint32_t
ncs_os_process_execute_timed(NCS_OS_PROC_EXECUTE_TIMED_INFO
*req)
* child */
if (getenv("OPENSAF_KEEP_FD_OPEN_AFTER_FORK") == NULL)
{
/* Close all inherited file descriptors */
- int i = sysconf(_SC_OPEN_MAX);
- if (i == -1) {
+ int fd_max = sysconf(_SC_OPEN_MAX);
+
+ if (fd_max == -1) {
syslog(LOG_ERR, "%s: sysconf failed - %s",
- __FUNCTION__, strerror(errno));
+ __FUNCTION__, strerror(errno));
exit(EXIT_FAILURE);
}
- for (i--; i >= 0; --i)
- (void)close(i); /* close all descriptors */
+ struct dirent *pentry = NULL;
+ DIR *dir = opendir("/proc/self/fd");
+
+ if (dir != NULL) {
+ while ((pentry = readdir(dir)) != NULL) {
[HansN] readdir will return not only 0, 1, 2 etc. but also the
current directory '.' and '..' this should be handled here.
[HansN] perhaps we should use readdir_r instead?
AndersW2> I also thought about this, but it turns out that
readdir_r is (or will become) obsolete. It is listed as obsolete
on our wiki page:
https://sourceforge.net/p/opensaf/wiki/Unsafe%20and%20Obsolete%20F
unctions/
Maybe we should unlist readdir() from the non-thread-safe section?
Regarding . and .., I think we should check for parse errors, i.e.
if it was a valid number or not. The second parameter to strtoimax
will (if not NULL) tell us how much of the string that was parsed.
It should point to a '\0' character if the whole string was parsed
as a valid number. In addition, you need to check that the string
was not empty to begin with.
I am thinking about adding a support function in base, that can
parse strings into numbers and handle parse errors in a convenient
way. the strtoxxx functions are a bit tricky since you need to
check the end pointer, and also errno for overflow/underflow errors.
+ int fd = strtoimax(pentry->d_name, NULL,
+10);
+ if (fd > INT_MIN && fd < fd_max)
(void)close(fd);
+ }
+ (void)closedir(dir);
+ } else {
+ /* fall back, close all possible descriptors */
+ syslog(LOG_ERR, "%s: opendir failed - %s",
+ __FUNCTION__, strerror(errno));
+ for (fd_max--; fd_max >= 0; --fd_max)
+ (void)close(fd_max);
+ }
/* Redirect standard files to /dev/null */
if (freopen("/dev/null", "r", stdin) == NULL)
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
