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:minh.c...@dektech.com.au] Sent: den 20 mars 2018 08:29 To: Hans Nordebäck <hans.nordeb...@ericsson.com>; Anders Widell <anders.wid...@ericsson.com>; ravisekhar.ko...@oracle.com Cc: opensaf-devel@lists.sourceforge.net 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 Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
