#48393 nsds5ReplicaProtocolTimeout attribute accepts negative values
Closed: fixed 2 years ago Opened 4 years ago by mreynolds.

Ticket was cloned from Red Hat Bugzilla (product Red Hat Enterprise Linux 7): Bug 1271208

Description of problem: Test case replica_timeout_05 in mmrepl/accept/accept.sh
is checking whether the replica entry accepts the negative(-15). The test case
fails since the server accepts the negative value. I encountered the same
problem when I manually executed the same tests.

[root@cloud-qe-4 export]# ldapmodify -x -p 24202 -h localhost -D "cn=Directory
Manager" -w Secret123 << EOF
> dn: cn=replica,cn=o\=airius.com,cn=mapping tree,cn=config
> changetype: modify
> replace: nsds5ReplicaProtocolTimeout
> nsds5ReplicaProtocolTimeout: -15
> EOF
modifying entry "cn=replica,cn=o\=airius.com,cn=mapping tree,cn=config"

Version-Release number of selected component (if applicable):
389-ds-base-1.3.4.0-19


How reproducible: Consistently

Steps to Reproduce:
1. Create a replication agreement or run mmrepl/accept tests from TET.
2. Check whether the server accepts negative value for
nsds5ReplicaProtocolTimeout attribute.
3. Check the test case replica_timeout_05 from TET acceptance mmraccept tests.

[root@cloud-qe-4 export]# ldapsearch -LLLx -p 24202 -h localhost -D
"cn=Directory Manager" -w Secret123 -b "cn=replica,cn=o\=airius.com,cn=mapping
tree,cn=config" nsds5ReplicaProtocolTimeout
dn: cn=replica,cn=o\3Dairius.com,cn=mapping tree,cn=config
nsds5ReplicaProtocolTimeout: -15

Actual results: Accepts negative value.

Expected results: It should reject the negative value with error 53.

[root@cloud-qe-4 export]# ldapmodify -x -p 24202 -h localhost -D "cn=Directory
Manager" -w Secret123 << EOFdn:
cn=24202_to_24206,cn=replica,cn=o\=airius.com,cn=mapping tree,cn=config
changetype: modify
replace: nsds5ReplicaProtocolTimeout
nsds5ReplicaProtocolTimeout: -15
EOF

modifying entry "cn=24202_to_24206,cn=replica,cn=o\=airius.com,cn=mapping
tree,cn=config"
ldap_modify: Server is unwilling to perform (53)
        additional info: attribute nsds5ReplicaProtocolTimeout value (-15) is
invalid, must be a number greater than zero.


Additional info:
Refer:
https://bugzilla.redhat.com/show_bug.cgi?id=918714
https://bugzilla.redhat.com/show_bug.cgi?id=1042855

Metadata Update from @mreynolds:
- Issue set to the milestone: 1.3.6.0

3 years ago

Metadata Update from @mreynolds:
- Issue close_status updated to: None
- Issue set to the milestone: 1.3.7 backlog (was: 1.3.6.0)

3 years ago

Metadata Update from @mreynolds:
- Issue assigned to mreynolds

2 years ago

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to review
- Custom field version adjusted to None

2 years ago

Forgot to check the repl agreement port number, new patch attached

0001-Ticket-48393-Improve-replication-config-validation.patch

I'm happy with this, but I think @lkrispen may have some input too ...

I think @lkrispen is away this week.

I'm going to nitpick your schema changes, can we make the DESC field actually describe what the attribute does?

Should repl_config_valid_num take int32_t not int? Actually, int64_t, because strtol is returing an int64_t not an int32_t, so the bounds check may not be complete there, and we can always extend int32_t to int64_t in cases we only accept the smaller value.

Sorry that I didn't review it closer on first pass,

I think @lkrispen is away this week.
I'm going to nitpick your schema changes, can we make the DESC field actually describe what the attribute does?
Should repl_config_valid_num take int32_t not int? Actually, int64_t, because strtol is returing an int64_t not an int32_t, so the bounds check may not be complete there, and we can always extend int32_t to int64_t in cases we only accept the smaller value.

I think the function is fine the way it is. Inside the function we use strtol, and set a int64_t variable (val). So the range is correctly checked. Then we use the min and max params to fine tune the range checking.

I do not see a "need" to change the int "return val" to int64_t, it would also require a lot of other changes in "a lot" of replication configuration code. It's a can of worms. Perhaps we can look into a change like this via a different ticket? I would really like to get this into 7.5 before the dev deadline.

Also there are already plans to redo all the schema descriptions, so I would also like to skip that for now as well.

Okay I just made all the changes, but it's more to review now

0001-Ticket-48393-Improve-replication-config-validation.patch

Everything looks great mate, except:

1689 +
1690 +int
1691 +repl_config_valid_num(const char *config_attr, char *config_attr_value, int min, int max,
1692 +                      int *returncode, char *errortext, int64_t *retval)
1693 +{

Because int min/max are int32_t, but val is int64_t, we can't express the range properly. :(

Everything looks great mate, except:
1689 +
1690 +int
1691 +repl_config_valid_num(const char config_attr, char config_attr_value, int min, int max,
1692 + int returncode, char errortext, int64_t *retval)
1693 +{

Because int min/max are int32_t, but val is int64_t, we can't express the range properly. :(

The CI test says otherwise ;-) If " val > max", even if max is a 32bit int, it still evaluates correctly, no?

On Fri, 2017-11-03 at 01:52 +0000, Mark Reynolds wrote:

mreynolds added a new comment to an issue you are following:
``

Everything looks great mate, except:
1689 +
1690 +int
1691 +repl_config_valid_num(const char config_attr, char
config_attr_value, int min, int max,
1692 +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int r=
eturncode, char
errortext,
int64_t *retval)
1693 +{
=20
=20
=20
Because int min/max are int32_t, but val is int64_t, we can't
express the range properly. :(
=20
The CI test says otherwise ;-)=C2=A0=C2=A0If " val > max", even if max is=
a
32bit int, it still evaluates correctly, no?
=20
=20

Yes, but if val > int32_t max, and we want a max of say ... greater
than that? It means we can't express the range int32_t max -> int64_t
max.=20

That's my point :)=20

IE 4294967295 -> 18446744073709551615L

:)=20

--=20
Sincerely,

William Brown
Software Engineer
Red Hat, Australia/Brisbane

Ps: email reply formatting does not look good on pagure .... :(

Anyway, if you change:

1690 +int
1691 +repl_config_valid_num(const char *config_attr, char *config_attr_value, int min, int max,
1692 +                      int *returncode, char *errortext, int64_t *retval)
1693 +{

to:

1690 +int
1691 +repl_config_valid_num(const char *config_attr, char *config_attr_value, int64_t min, int64_
t max,
1692 +                      int *returncode, char *errortext, int64_t *retval)
1693 +{

Then you have my ack :)

On Fri, 2017-11-03 at 01:52 +0000, Mark Reynolds wrote:

mreynolds added a new comment to an issue you are following:
``

Everything looks great mate, except:
1689 +
1690 +int
1691 +repl_config_valid_num(const char config_attr, char
config_attr_value, int min, int max,
1692 +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int r=
eturncode, char errortext,
int64_t *retval)
1693 +{
=20
=20
=20
Because int min/max are int32_t, but val is int64_t, we can't
express the range properly. :(
=20
The CI test says otherwise ;-)=C2=A0=C2=A0If " val > max", even if max is=
a
32bit int, it still evaluates correctly, no?
=20
=20

Yes, but if val > int32_t max, and we want a max of say ... greater
than that?

Ahh, I should of mentioned this sooner. Technically max is not supposed to be greater than an 32bit int according to the official docs. All our config values are not supposed to exceed a 32 bit int. So I was originally trying to be compliant with the docs.

Note the CI test does check "all" of this and does pass. Anyway I'll change it...

Revised patch

0001-Ticket-48393-Improve-replication-config-validation.patch

acking for William..

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to ack (was: review)

2 years ago

fe85945..f6b0e18 master -> master

8ca21dd..1b04a2d 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

Metadata Update from @mreynolds:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

2 years ago

Copy and paste error detected by covscan:

e1f866a..4316470 master -> master

b94a648..578e8a1 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

A 64bit int can hold a 32bit one :) And we may choose to expand that in the future :)

Anyway ,thank you so much for listening to my comments!

Metadata Update from @spichugi:
- Custom field reviewstatus adjusted to review (was: ack)

2 years ago

Metadata Update from @spichugi:
- Custom field reviewstatus adjusted to None (was: review)

2 years ago

Login to comment on this ticket.

Metadata