Re: [PATCH] CLEANUP: ssl: remove opendir call in ssl_sock_load_cert

[William Dauchy]

Hi William L.,

Thanks for your  answer.

On Mon, Jan 13, 2020 at 04:00:53PM +0100, William Lallemand wrote:
> I understand that you were trying to remove opendir, which is good idea.
> However, I find it kind of confusing: if ssl_sock_load_ckchs() returns an
> error, this error will be added to the "unable to scan directory" message, and
> at this point it is not trying to scan a directory anymore.
> 
> However, there is already a call to stat(), we could probably skip the
> call to opendir() by checking S_IFDIR in the stat structure.

True, I probably over engineered it, I've sent v2, which is simpler.

Thanks,
-- 
William

Re: [PATCH] CLEANUP: ssl: remove opendir call in ssl_sock_load_cert

[William Lallemand]

Hello,

I understand that you were trying to remove opendir, which is good idea.
However, I find it kind of confusing: if ssl_sock_load_ckchs() returns an
error, this error will be added to the "unable to scan directory" message, and
at this point it is not trying to scan a directory anymore.

However, there is already a call to stat(), we could probably skip the
call to opendir() by checking S_IFDIR in the stat structure.


>   n = scandir(path, _list, 0, alphasort);
>   if (n < 0) {
> - memprintf(err, "%sunable to scan directory '%s' : 
> %s.\n",
> -   err && *err ? *err : "", path, 
> strerror(errno));
> - cfgerr |= ERR_ALERT | ERR_FATAL;
> - }
> - else {
> + memprintf(err, "%sunable to scan directory '%s': %s; "
> + "trying to load as file.\n",
> + err && *err ? *err : "", path, 
> strerror(errno));
> + ckchs = ckchs_load_cert_file(path, 0,  err);
> + if (!ckchs)
> + return ERR_ALERT | ERR_FATAL;
> + return ssl_sock_load_ckchs(path, ckchs, bind_conf, 
> NULL, NULL, 0, err);
> + } else {

-- 
William Lallemand

[PATCH] CLEANUP: ssl: remove opendir call in ssl_sock_load_cert

[William Dauchy]

Since commit 3180f7b55434 ("MINOR: ssl: load certificates in
alphabetical order"), `readdir` was replaced by `scandir`. I first
wanted to simply move the `closedir` earlier as we no longer use it
after, but then I thought we could simply rely on `scandir` return
value; in case of error, it could be:
- not a directory, in that case we try to load it as file, as before
- any other error, in which case `ckchs_load_cert_file` will fail as
well

Signed-off-by: William Dauchy 
---
 src/ssl_sock.c | 24 
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index e4dd913a..3e8d57c2 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -4249,7 +4249,6 @@ int ssl_sock_load_cert(char *path, struct bind_conf 
*bind_conf, char **err)
 {
struct dirent **de_list;
int i, n;
-   DIR *dir;
struct stat buf;
char *end;
char fp[MAXPATHLEN+1];
@@ -4265,26 +4264,20 @@ int ssl_sock_load_cert(char *path, struct bind_conf 
*bind_conf, char **err)
}
 
if (stat(path, ) == 0) {
-   dir = opendir(path);
-   if (!dir) {
-   ckchs =  ckchs_load_cert_file(path, 0,  err);
-   if (!ckchs)
-   return ERR_ALERT | ERR_FATAL;
-
-   return ssl_sock_load_ckchs(path, ckchs, bind_conf, 
NULL, NULL, 0, err);
-   }
-
/* strip trailing slashes, including first one */
for (end = path + strlen(path) - 1; end >= path && *end == '/'; 
end--)
*end = 0;
 
n = scandir(path, _list, 0, alphasort);
if (n < 0) {
-   memprintf(err, "%sunable to scan directory '%s' : 
%s.\n",
- err && *err ? *err : "", path, 
strerror(errno));
-   cfgerr |= ERR_ALERT | ERR_FATAL;
-   }
-   else {
+   memprintf(err, "%sunable to scan directory '%s': %s; "
+   "trying to load as file.\n",
+   err && *err ? *err : "", path, 
strerror(errno));
+   ckchs = ckchs_load_cert_file(path, 0,  err);
+   if (!ckchs)
+   return ERR_ALERT | ERR_FATAL;
+   return ssl_sock_load_ckchs(path, ckchs, bind_conf, 
NULL, NULL, 0, err);
+   } else {
for (i = 0; i < n; i++) {
struct dirent *de = de_list[i];
 
@@ -4355,7 +4348,6 @@ ignore_entry:
}
free(de_list);
}
-   closedir(dir);
return cfgerr;
}
 
-- 
2.24.1