There is one issue I am having with it that I need a help to figure out.

All the code is ready and in place except for one thing. I need to run a script at the shutdown. Currently, I've put the code to the end of cleanup right before return. By some reason, system() function is executed but it always returns a success code (I've also tried fork-execl combination). But the script is actually is not executed...

I've tried the same code in a separate .c file that I compiled locally and it works.

Do I miss something that happens during the nsslapd shutdown process that can intervene with my script execution?
Or what will be the better place to put my shutdown script code, what do you think?

This seems risky to just "run" given it relies on filling disk capacity .... I have a 1TB ssd, I don't exactly want to see what happens when this runs >.<

size_t for array offset.

Why are you dividing by /256 here?

https://linux.die.net/man/3/system shows it should just return the code?

Anyway, I think that this is all fine except the "run a script on shutdown". That's a possible remote code execution vector. As much as we don't want it to happen, this presents a possible chain of -> cn=directory manager -> change cn=config -> access to the machine. Saying this it should be "unlikely" because cn=directory manager gives you access to change the cn=admin or other users who could login, but say a shared hosting or high security env? Maybe the directory server is not configured to use ldap to attempt to harden it?

So honestly, I like the sentiment, but I think I'm not comfortable with an arbitrary script being run here. You should consider discussing this with product security at redhat ...

This seems risky to just "run" given it relies on filling disk capacity .... I have a 1TB ssd, I don't exactly want to see what happens when this runs >.<

As a setup, it mounts tmpfs (35M). And we use 30000000 threshold. So it should be okay for any Linux system you test on.
But the whole test needs refactoring... I agree...

size_t for array offset.

gonna fix it, thanks!

Why are you dividing by /256 here?

because system() returns the exit code from https://linux.die.net/man/2/wait and it returns WEXITSTATUS(status) macro if the child is exited. Which consists of the least significant 8 bits of the status

realloc looks weird on a NULL pointer. If you decide to limit the #backend to BACKEND_SIZE, why not

Slapi_Backend *be_list[BACKEND_SIZE] = {0;

BE_LIST_SIZE and BACKEND_SIZE should be aligned else it will sigsev.
An other option would be to use a real list of Slapi_Backend, so we can ignore limits

Switching all backends to readonly looks fine to me. To be more flexible we could switch readonly only the backends that have files in full file system.
The comments could mention that we are switching all of them.

slapi_be_free should be called for each backend in be_list.
then slapi_ch_free(&be_list);

same as above. It should first free all be in be_list, then free be_list

Did you test that after disk threshold shutdown, at next restart the backends are not in readonly

Anyway, I think that this is all fine except the "run a script on shutdown".

Yeah, I agree. I wanted to discuss the approach and possible options and I think it is better to have some code before eyes while discussing.
The main RFE goal is to minimize the risk of data corruption while dealing with disk monitoring. One customer asked for that.

The script is an additional request. And after some more thinking and discussing with colleagues, I've decided to remove the script from this PR and make a proposal for the customer to resolve the issue another way (run their script in systemd service or add some cron job).

realloc looks weird on a NULL pointer. If you decide to limit the #backend to BACKEND_SIZE, why not
Slapi_Backend *be_list[BACKEND_SIZE] = {0;

Fixed.

Switching all backends to readonly looks fine to me. To be more flexible we could switch readonly only the backends that have files in full file system.
The comments could mention that we are switching all of them.

I think it is less complex code and it'll be more robust to have everything in read-only... I don't see the options when somebody needs a backend without files (except Schema and Config). So if the files will appear later and we have the read-write backend - we will need additionally process that case. So I think it's better to put everything user made to read-only to begin with.

Or maybe I miss something?

slapi_be_free should be called for each backend in be_list.
then slapi_ch_free(&be_list);

Done.

Did you test that after disk threshold shutdown, at next restart the backends are not in readonly

Yep, it is in both test_readonly_on_threshold_below_half_of_the_threshold and test_readonly_on_threshold tests.

Changes are done. Please, review...

This seems risky to just "run" given it relies on filling disk capacity .... I have a 1TB ssd, I don't exactly want to see what happens when this runs >.<

As a setup, it mounts tmpfs (35M). And we use 30000000 threshold. So it should be okay for any Linux system you test on.

I won't be able to do this in containers that don't have full system privileges. Can you put this test behind an envirconment variable (similar to GSSAPI_ACK)?

because system() returns the exit code from https://linux.die.net/man/2/wait and it returns WEXITSTATUS(status) macro if the child is exited. Which consists of the least significant 8 bits of the status

You may want to bit mask instead then ....

Anyway, I think that this is all fine except the "run a script on shutdown".

Yeah, I agree. I wanted to discuss the approach and possible options and I think it is better to have some code before eyes while discussing.
The main RFE goal is to minimize the risk of data corruption while dealing with disk monitoring. One customer asked for that.
The script is an additional request. And after some more thinking and discussing with colleagues, I've decided to remove the script from this PR and make a proposal for the customer to resolve the issue another way (run their script in systemd service or add some cron job).

I think we should make this a seperate feature or investigation. I have a few ideas on what could be possible, but pehaps something like a dbus notification system could be better, and there is a whole raft of possible things we could notify on, not just this.

Anyway, thanks for the feature, it looks good mate (personal note, super exciting to see you doing some more C code :D )

Anyway, I think that this is all fine except the "run a script on shutdown".
Yeah, I agree. I wanted to discuss the approach and possible options and I think it is better to have some code before eyes while discussing.
The main RFE goal is to minimize the risk of data corruption while dealing with disk monitoring. One customer asked for that.
The script is an additional request. And after some more thinking and discussing with colleagues, I've decided to remove the script from this PR and make a proposal for the customer to resolve the issue another way (run their script in systemd service or add some cron job).

I think we should make this a seperate feature or investigation. I have a few ideas on what could be possible, but pehaps something like a dbus notification system could be better, and there is a whole raft of possible things we could notify on, not just this.
Anyway, thanks for the feature, it looks good mate (personal note, super exciting to see you doing some more C code :D )

I had similar concern while reading the patch but convince myself because the script will be executed with the 'nsslapd-localuser' rights. So even if the script does dangerous things or is replaced by an attacker it will only do what nsslapd-localuser can do. Checking that nsslapd-localuser is not root could it be a solution ?

I generally tend to agree with guys that the shutdown script is more of an external setup thing like cron job, etc.
The customer has requested the script for things such - take a backup, send an email to admin, do some additional actions with external services, etc.

So yes, we can implement it with the dbus, or check its selinux context, or nsslapd-localuser, etc.
I just don't think we should do this especially for the disk monitoring feature and for that customer case.

But we can do it as a separate thing as William proposed with a separate investigation (i.e. maybe it can be used by other DS components).

What do you think?

I won't be able to do this in containers that don't have full system privileges. Can you put this test behind an envirconment variable (similar to GSSAPI_ACK)?

Done.

because system() returns the exit code from https://linux.die.net/man/2/wait and it returns WEXITSTATUS(status) macro if the child is exited. Which consists of the least significant 8 bits of the status

You may want to bit mask instead then ....

Sure, it was a common example with / 256 so I just used it. But thinking more now I agree that shifting 8 bites will be a more smart approach.
Anyway, this is a part of the code that is removed.

Please, review.

I had similar concern while reading the patch but convince myself because the script will be executed with the 'nsslapd-localuser' rights. So even if the script does dangerous things or is replaced by an attacker it will only do what nsslapd-localuser can do. Checking that nsslapd-localuser is not root could it be a solution ?

Linux local system security is swiss cheese - you must assume any user access can be trivially escalated to root. So I think this needs to be seperate. We really should look at a better way to do notifications, that doesn't involve code exec.

I generally tend to agree with guys that the shutdown script is more of an external setup thing like cron job, etc.
The customer has requested the script for things such - take a backup, send an email to admin, do some additional actions with external services, etc.
So yes, we can implement it with the dbus, or check its selinux context, or nsslapd-localuser, etc.
I just don't think we should do this especially for the disk monitoring feature and for that customer case.
But we can do it as a separate thing as William proposed with a separate investigation (i.e. maybe it can be used by other DS components).
What do you think?

Lets do it in a seperate issue, otherwise the rest of this looks good to me :) Thanks so much @spichugi

I would prefer to have a comment saying all backends are move to readonly.
Some file system, hosting backend, are possibly not full but we switch them readonly as well.
Only exception are in memory backend dse,schema, defaut_backend.

prefer "Slapi_Backend *be_list[BE_LIST_SIZE] = {0};

The overall looks nice but I have a concern about be_list_count.
It is sometime used as a local variable (for example to free backends) but it is global to the main loop and used at different places without knowing its initial expected value at those places.

many loops using be_list and be_list_count are not checking BE_LIST_SIZE limit.
You may test BE_LIST_SIZE or add a final always NULL slot be_list[BE_LIST_SIZE] = NULL.

Fixed. Please, review

be_list is now a local variable, it is no longer malloc'ed. do not slapi_ch_free it.

There is a possibility that at this place be_list_count == BE_LIST_SIZE.
be_list[BE_LIST_SIZE] overflow the buffer. You may test be_list_count before using it

be_list is now a local variable, it is no longer malloc'ed. do not slapi_ch_free it.

Right, missed that.

There is a possibility that at this place be_list_count == BE_LIST_SIZE.
be_list[BE_LIST_SIZE] overflow the buffer. You may test be_list_count before using it

Yep, now it's fixed consistently with the same issue solution in opshared.c.

Please, check!

sorry missed that one. slapi_ch_free((void **)&be_list); should be removed

@spichugi thanks for your patience. You have my ACK

Thank you for the help and the great review :)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4014

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.