Hi Minh, Yes, that will be ok. /Regards HansN
-----Original Message----- From: Minh Hon Chau [mailto:minh.c...@dektech.com.au] Sent: den 20 mars 2018 12:53 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, 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: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%20 >>>>>> F >>>>>> 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
