#50395 Ticket 50393 - maxlogsperdir accepting negative values
Merged 11 months ago by mreynolds. Opened 11 months ago by mreynolds.
mreynolds/389-ds-base ticket50393  into  master

@@ -0,0 +1,3 @@ 

+ """

+    :Requirement: 389-ds-base: Directory Server Logging Configurations

+ """

@@ -0,0 +1,86 @@ 

+ import logging

+ import pytest

+ import os

+ import ldap

+ from lib389._constants import *

+ from lib389.topologies import topology_st as topo

+ 

+ DEBUGGING = os.getenv("DEBUGGING", default=False)

+ if DEBUGGING:

+     logging.getLogger(__name__).setLevel(logging.DEBUG)

+ else:

+     logging.getLogger(__name__).setLevel(logging.INFO)

+ log = logging.getLogger(__name__)

+ 

+ big_value = "1111111111111111111111111111111111111111111"

+ 

+ 

+ @pytest.mark.parametrize("attr, invalid_vals, valid_vals",

+                          [

+                              ("logexpirationtime", ["-2", "0"], ["1", "-1"]),

+                              ("maxlogsize", ["-2", "0"], ["100", "-1"]),

+                              ("logmaxdiskspace", ["-2", "0"], ["100", "-1"]),

+                              ("logminfreediskspace", ["-2", "0"], ["100", "-1"]),

+                              ("mode", ["888", "778", "77", "7777"], ["777", "000", "600"]),

+                              ("maxlogsperdir", ["-1", "0"], ["1", "20"]),

+                              ("logrotationsynchour", ["-1", "24"], ["0", "23"]),

+                              ("logrotationsyncmin", ["-1", "60"], ["0", "59"]),

+                              ("logrotationtime", ["-2", "0"], ["100", "-1"])

+                          ])

+ def test_logging_digit_config(topo, attr, invalid_vals, valid_vals):

+     """Validate logging config settings

+ 

+     :id: a0ef30e5-538b-46fa-9762-01a4435a15e9

+     :setup: Standalone Instance

+     :steps:

+         1. Test log expiration time

+         2. Test log max size

+         3. Test log max disk space

+         4. Test log min disk space

+         5. Test log mode

+         6. Test log max number of logs

+         7. Test log rotation hour

+         8. Test log rotation minute

+         9. Test log rotation time

+     :expectedresults:

+         1. Success

+         2. Success

+         3. Success

+         4. Success

+         5. Success

+         6. Success

+         7. Success

+         8. Success

+         9. Success

+     """

+ 

+     accesslog_attr = "nsslapd-accesslog-{}".format(attr)

+     auditlog_attr = "nsslapd-auditlog-{}".format(attr)

+     auditfaillog_attr = "nsslapd-auditfaillog-{}".format(attr)

+     errorlog_attr = "nsslapd-errorlog-{}".format(attr)

+ 

+     # Test each log

+     for attr in [accesslog_attr, auditlog_attr, auditfaillog_attr, errorlog_attr]:

+         # Invalid values

+         for invalid_val in invalid_vals:

+             with pytest.raises(ldap.LDAPError):

+                 topo.standalone.config.set(attr, invalid_val)

+ 

+         # Invalid high value

+         with pytest.raises(ldap.LDAPError):

+             topo.standalone.config.set(attr, big_value)

+ 

+         # Non digits

+         with pytest.raises(ldap.LDAPError):

+             topo.standalone.config.set(attr, "abc")

+ 

+         # Valid values

+         for valid_val in valid_vals:

+             topo.standalone.config.set(attr, valid_val)

+ 

+ 

+ if __name__ == '__main__':

+     # Run isolated

+     # -s for DEBUG mode

+     CURRENT_FILE = os.path.realpath(__file__)

+     pytest.main(["-s", CURRENT_FILE])

file modified
+103 -40

@@ -817,8 +817,9 @@ 

  int

  log_set_mode(const char *attrname, char *value, int logtype, char *errorbuf, int apply)

  {

-     int v = 0;

+     int64_t v = 0;

      int retval = LDAP_SUCCESS;

+     char *endp = NULL;

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (NULL == value) {

@@ -833,7 +834,18 @@ 

          return LDAP_SUCCESS;

      }

  

-     v = strtol(value, NULL, 8);

+     errno = 0;

+     v = strtol(value, &endp, 8);

+     if (*endp != '\0' || errno == ERANGE ||

+         strlen(value) != 3 ||

+         v > 0777 /* octet of 777 511 */ ||

+         v < 0)

+     {

+         slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s) (%ld), value must be three digits between 000 and 777",

+                 value, attrname, v);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

  

      switch (logtype) {

      case SLAPD_ACCESS_LOG:

@@ -895,9 +907,9 @@ 

  log_set_numlogsperdir(const char *attrname, char *numlogs_str, int logtype, char *returntext, int apply)

  {

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

- 

+     char *endp = NULL;

      int rv = LDAP_SUCCESS;

-     int numlogs;

+     int64_t numlogs;

  

      if (logtype != SLAPD_ACCESS_LOG &&

          logtype != SLAPD_ERROR_LOG &&

@@ -911,7 +923,14 @@ 

          return rv;

      }

  

-     numlogs = atoi(numlogs_str);

+     errno = 0;

+     numlogs = strtol(numlogs_str, &endp, 10);

+     if (*endp != '\0' || errno == ERANGE || numlogs < 1) {

+         PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s), value must be between 1 and 2147483647",

+                 numlogs_str, attrname);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

  

      if (numlogs >= 1) {

          switch (logtype) {

@@ -960,21 +979,25 @@ 

  log_set_logsize(const char *attrname, char *logsize_str, int logtype, char *returntext, int apply)

  {

      int rv = LDAP_SUCCESS;

-     PRInt64 max_logsize;    /* in bytes */

-     int logsize;            /* in megabytes */

+     int64_t max_logsize; /* in bytes */

+     int64_t logsize;     /* in megabytes */

+     char *endp = NULL;

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (!apply || !logsize_str || !*logsize_str)

          return rv;

  

-     logsize = atoi(logsize_str);

+     errno = 0;

+     logsize = strtol(logsize_str, &endp, 10);

+     if (*endp != '\0' || errno == ERANGE || logsize < -1 || logsize == 0) {

+         PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than 0",

+                 logsize_str, attrname);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

  

      /* convert it to bytes */

-     max_logsize = (PRInt64)logsize * LOG_MB_IN_BYTES;

- 

-     if (max_logsize <= 0) {

-         max_logsize = -1;

-     }

+     max_logsize = logsize * LOG_MB_IN_BYTES;

  

      switch (logtype) {

      case SLAPD_ACCESS_LOG:

@@ -1101,8 +1124,9 @@ 

  int

  log_set_rotationsynchour(const char *attrname, char *rhour_str, int logtype, char *returntext, int apply)

  {

-     int rhour = -1;

+     int64_t rhour = -1;

      int rv = LDAP_SUCCESS;

+     char *endp = NULL;

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (logtype != SLAPD_ACCESS_LOG &&

@@ -1115,12 +1139,19 @@ 

      }

  

      /* return if we aren't doing this for real */

-     if (!apply) {

+     if (!apply || !rhour_str || !*rhour_str) {

          return rv;

      }

  

-     if (rhour_str && *rhour_str != '\0')

-         rhour = atol(rhour_str);

+     errno = 0;

+     rhour = strtol(rhour_str, &endp, 10);

+     if (*endp != '\0' || errno == ERANGE || rhour < 0 || rhour > 23) {

+         PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s), value must be \"0\" thru \"23\"",

+                 rhour_str, attrname);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

+ 

      if (rhour > 23)

          rhour = rhour % 24;

  

@@ -1161,8 +1192,9 @@ 

  int

  log_set_rotationsyncmin(const char *attrname, char *rmin_str, int logtype, char *returntext, int apply)

  {

-     int rmin = -1;

+     int64_t rmin = -1;

      int rv = LDAP_SUCCESS;

+     char *endp = NULL;

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (logtype != SLAPD_ACCESS_LOG &&

@@ -1175,14 +1207,18 @@ 

      }

  

      /* return if we aren't doing this for real */

-     if (!apply) {

+     if (!apply || !rmin_str || !*rmin_str) {

          return rv;

      }

  

-     if (rmin_str && *rmin_str != '\0')

-         rmin = atol(rmin_str);

-     if (rmin > 59)

-         rmin = rmin % 60;

+     errno = 0;

+     rmin = strtol(rmin_str, &endp, 10);

+     if (*endp != '\0' || errno == ERANGE || rmin < 0 || rmin > 59) {

+         PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s), value must be between \"0\" and \"59\"",

+                 rmin_str, attrname);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

  

      switch (logtype) {

      case SLAPD_ACCESS_LOG:

@@ -1229,8 +1265,9 @@ 

  {

  

      int runit = 0;

-     int value, rtime;

+     int64_t value, rtime;

      int rv = LDAP_SUCCESS;

+     char *endp = NULL;

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (logtype != SLAPD_ACCESS_LOG &&

@@ -1247,7 +1284,14 @@ 

          return rv;

      }

  

-     rtime = atoi(rtime_str);

+     errno = 0;

+     rtime = strtol(rtime_str, &endp, 10);

+     if (*endp != '\0' || errno == ERANGE || rtime < -1 || rtime == 0) {

+         PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than \"0\"",

+                 rtime_str, attrname);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

  

      if (0 == rtime) {

          rtime = -1; /* Value Range: -1 | 1 to PR_INT32_MAX */

@@ -1332,7 +1376,6 @@ 

      int origvalue = 0, value = 0;

      int runitType;

      int rv = 0;

- 

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (logtype != SLAPD_ACCESS_LOG &&

@@ -1448,10 +1491,10 @@ 

  log_set_maxdiskspace(const char *attrname, char *maxdiskspace_str, int logtype, char *errorbuf, int apply)

  {

      int rv = 0;

-     PRInt64 mlogsize = 0; /* in bytes */

-     PRInt64 maxdiskspace; /* in bytes */

-     int s_maxdiskspace;   /* in megabytes */

- 

+     int64_t mlogsize = 0;   /* in bytes */

+     int64_t maxdiskspace;   /* in bytes */

+     int64_t s_maxdiskspace; /* in megabytes */

+     char *endp = NULL;

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (logtype != SLAPD_ACCESS_LOG &&

@@ -1465,7 +1508,14 @@ 

      if (!apply || !maxdiskspace_str || !*maxdiskspace_str)

          return rv;

  

-     s_maxdiskspace = atoi(maxdiskspace_str);

+     errno = 0;

+     s_maxdiskspace = strtol(maxdiskspace_str, &endp, 10);

+     if (*endp != '\0' || errno == ERANGE || s_maxdiskspace < -1 || s_maxdiskspace == 0) {

+         slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than 0",

+                 maxdiskspace_str, attrname);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

  

      /* Disk space are in MB  but store in bytes */

      switch (logtype) {

@@ -1538,9 +1588,9 @@ 

  log_set_mindiskspace(const char *attrname, char *minfreespace_str, int logtype, char *errorbuf, int apply)

  {

      int rv = LDAP_SUCCESS;

-     int minfreespace;      /* in megabytes */

-     PRInt64 minfreespaceB; /* in bytes */

- 

+     int64_t minfreespace;  /* in megabytes */

+     int64_t minfreespaceB; /* in bytes */

+     char *endp = NULL;

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (logtype != SLAPD_ACCESS_LOG &&

@@ -1556,11 +1606,18 @@ 

          return rv;

      }

  

-     minfreespace = atoi(minfreespace_str);

+     errno = 0;

+     minfreespace = strtol(minfreespace_str, &endp, 10);

+     if (*endp != '\0' || errno == ERANGE || minfreespace < -1 || minfreespace == 0) {

+         slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than 0",

+                 minfreespace_str, attrname);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

  

      /* Disk space are in MB  but store in bytes */

      if (minfreespace >= 1) {

-         minfreespaceB = (PRInt64)minfreespace * LOG_MB_IN_BYTES;

+         minfreespaceB = minfreespace * LOG_MB_IN_BYTES;

          switch (logtype) {

          case SLAPD_ACCESS_LOG:

              LOG_ACCESS_LOCK_WRITE();

@@ -1602,10 +1659,10 @@ 

  int

  log_set_expirationtime(const char *attrname, char *exptime_str, int logtype, char *errorbuf, int apply)

  {

- 

-     int eunit, value, exptime;

+     int64_t eunit, value, exptime;

      int rsec = 0;

      int rv = 0;

+     char *endp = NULL;

      slapdFrontendConfig_t *fe_cfg = getFrontendConfig();

  

      if (logtype != SLAPD_ACCESS_LOG &&

@@ -1621,7 +1678,14 @@ 

          return rv;

      }

  

-     exptime = atoi(exptime_str); /* <= 0: no exptime */

+     errno = 0;

+     exptime = strtol(exptime_str, &endp, 10);

+     if (*endp != '\0' || errno == ERANGE || exptime < -1 || exptime == 0) {

+         slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE,

+                 "Invalid value \"%s\" for attribute (%s), value must be \"-1\" or greater than 0",

+                 exptime_str, attrname);

+         return LDAP_UNWILLING_TO_PERFORM;

+     }

  

      switch (logtype) {

      case SLAPD_ACCESS_LOG:

@@ -1734,7 +1798,6 @@ 

      } else {

          slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE, "%s: invalid time unit \"%s\"", attrname, expunit);

          rv = LDAP_OPERATIONS_ERROR;

-         ;

      }

  

      /* return if we aren't doing this for real */

Description: Improve the log "digit" config setting validation for all settings.

https://pagure.io/389-ds-base/issue/50393

Patch looks good.
minor comment, reading octal, '777' will be 511 decimal. (v>511)

Probably the between word should be omitted here.

Patch looks good.
minor comment, reading octal, '777' will be 511 decimal. (v>511)

Ahh, good point. I will revise those numbers accordingly...

In the test we could use @pytest.mark.parametrize(...) instead of calling a separate function repeatedly. The resulting logs of test runs would be better readable later on.

In the test we could use @pytest.mark.parametrize(...) instead of calling a separate function repeatedly. The resulting logs of test runs would be better readable later on.

I did think of that, but I personally think it is harder to read those big lumps of parameters. I can change it I suppose...

In the test we could use @pytest.mark.parametrize(...) instead of calling a separate function repeatedly. The resulting logs of test runs would be better readable later on.

I did think of that, but I personally think it is harder to read those big lumps of parameters. I can change it I suppose...

That was just a suggestion :), the code is well readable anyways. But with the parametrize, all the cases would be run separately, so one assert fail would not result in not running the rest of attributes' checks, as an added benefit.

1 new commit added

  • Revised mode octet range checking, and paramrtrized the CI test
11 months ago

@tbordaz, @mhonek - changes made please review...

rebased onto 36e627fdee71787fc1a142b0f2b2f9d12e16a545

11 months ago

rebased onto fe2f69b8b93a76526e5dc70eb5c96ede73db95ec

11 months ago

strlen could be your first check after the endp != NULL, given it's probably the most likely to be bad.

Just a nit pick... :) Now we could just replace the call with the function's contents.

511 could be replaced by 0777 here, where the 0 implies octal number.

Superfluous space character after word be.

One more superfluous word between.

The upper bound 60 here is excluded, unlike in other log messages where we include the upper boundary number. Notice the > 59 in the condition, which should be > 60, I think.

Edit to my last comment: from the context it seems the condition is actually OK, just the log message should be probably changed to show between 0 and 59.

In the test we could use @pytest.mark.parametrize(...) instead of calling a separate function repeatedly. The resulting logs of test runs would be better readable later on.

I did think of that, but I personally think it is harder to read those big lumps of parameters. I can change it I suppose...

Parameters can be assigned with human readable IDs: https://docs.pytest.org/en/latest/example/parametrize.html (see "Different options for test IDs")

Edit to my last comment: from the context it seems the condition is actually OK, just the log message should be probably changed to show between 0 and 59.

Right the wording in the message is misleading, I'll get these things changed shortly...

1 new commit added

  • Revise CI test, and minor changes to DS log messages
11 months ago

3 new commits added

  • Revise CI test, and minor changes to DS log messages
  • Revised mode octet range checking, and paramrtrized the CI test
  • Ticket 50393 - maxlogsperdir accepting negative values
11 months ago

Changes made please review

1 new commit added

  • Fix another log message
11 months ago

rebased onto 45b8177a068019067627906ad186d43ab5f0a68a

11 months ago

rebased onto ca70d06

11 months ago

Pull-Request has been merged by mreynolds

11 months ago