[PATCH] libsemanage: properly check return value of iterate function

Discussion:

Jan Zarsky

2017-11-22 15:09:59 UTC

Function dbase_llist_iterate iterates over records and checks return
value of iterate function. According to a manpage semanage_iterate(3),
handler can return value 1 for early exit. dbase_llist_iterate
currently checks for return value > 1, which does not include
expected value 1. This affects most of the semanage_*_iterate
and semanage_*_local functions.

Signed-off-by: Jan Zarsky <***@redhat.com>
---
libsemanage/src/database_llist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsemanage/src/database_llist.c b/libsemanage/src/database_llist.c
index 8ce2e2c1..c8f4ff0b 100644
--- a/libsemanage/src/database_llist.c
+++ b/libsemanage/src/database_llist.c
@@ -263,7 +263,7 @@ int dbase_llist_iterate(semanage_handle_t * handle,
if (rc < 0)
goto err;

- else if (rc > 1)
+ else if (rc > 0)
break;
}

--
2.14.3

William Roberts

2017-11-22 21:35:05 UTC

Permalink

Can you update this message to describe what is affected.

Post by Jan Zarsky
---
libsemanage/src/database_llist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libsemanage/src/database_llist.c b/libsemanage/src/database_llist.c
index 8ce2e2c1..c8f4ff0b 100644
--- a/libsemanage/src/database_llist.c
+++ b/libsemanage/src/database_llist.c
@@ -263,7 +263,7 @@ int dbase_llist_iterate(semanage_handle_t * handle,
if (rc < 0)
goto err;
- else if (rc > 1)
+ else if (rc > 0)

This looks fine to me.

Post by Jan Zarsky
break;
}
--
2.14.3

Please resend with the message updated and I'll ack.

Jan Zarsky

2017-11-27 10:01:02 UTC

Permalink

Function dbase_llist_iterate() iterates over records and checks return
value of iterate function. According to a manpage semanage_iterate(3),
handler can return value 1 for early exit. dbase_llist_iterate()
currently checks for return value > 1, which does not include
expected value 1.

Affected functions:
semanage_bool_iterate_local
semanage_fcontext_iterate
semanage_fcontext_iterate_local
semanage_ibendport_iterate_local
semanage_ibpkey_iterate_local
semanage_iface_iterate_local
semanage_node_iterate_local
semanage_port_iterate_local
semanage_seuser_iterate
semanage_seuser_iterate_local
semanage_user_iterate
semanage_user_iterate_local

Signed-off-by: Jan Zarsky <***@redhat.com>
---
libsemanage/src/database_llist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsemanage/src/database_llist.c b/libsemanage/src/database_llist.c
index 8ce2e2c1..c8f4ff0b 100644
--- a/libsemanage/src/database_llist.c
+++ b/libsemanage/src/database_llist.c
@@ -263,7 +263,7 @@ int dbase_llist_iterate(semanage_handle_t * handle,
if (rc < 0)
goto err;

- else if (rc > 1)
+ else if (rc > 0)
break;
}

--
2.14.3

William Roberts

2017-11-27 15:50:27 UTC

Permalink

Post by Jan Zarsky
Function dbase_llist_iterate() iterates over records and checks return
value of iterate function. According to a manpage semanage_iterate(3),
handler can return value 1 for early exit. dbase_llist_iterate()
currently checks for return value > 1, which does not include
expected value 1.
semanage_bool_iterate_local
semanage_fcontext_iterate
semanage_fcontext_iterate_local
semanage_ibendport_iterate_local
semanage_ibpkey_iterate_local
semanage_iface_iterate_local
semanage_node_iterate_local
semanage_port_iterate_local
semanage_seuser_iterate
semanage_seuser_iterate_local
semanage_user_iterate
semanage_user_iterate_local

Not really what I had in mind. I meant what was the affect. This is simple
enough to gather, so ack on v1,

My understanding is that the affect is that it that it doesn't short
circuit the iterate
routine so lockups take longer than they need be, is that correct?

--
Respectfully,

William C Roberts

Jan Zarsky

2017-12-11 13:47:53 UTC

Permalink

----- Original Message -----

Post by William Roberts

Not really what I had in mind. I meant what was the affect. This is simple
enough to gather, so ack on v1,
My understanding is that the affect is that it that it doesn't short
circuit the iterate
routine so lockups take longer than they need be, is that correct?

Yes, that is the exactly the problem. I will try to be more clear next time.

Post by William Roberts

Post by Jan Zarsky
---
libsemanage/src/database_llist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libsemanage/src/database_llist.c
b/libsemanage/src/database_llist.c
index 8ce2e2c1..c8f4ff0b 100644
--- a/libsemanage/src/database_llist.c
+++ b/libsemanage/src/database_llist.c
@@ -263,7 +263,7 @@ int dbase_llist_iterate(semanage_handle_t * handle,
if (rc < 0)
goto err;
- else if (rc > 1)
+ else if (rc > 0)
break;
}
--
2.14.3

--
Respectfully,
William C Roberts

William Roberts

2017-11-27 19:09:44 UTC

Permalink

Thanks. Applied: https://github.com/SELinuxProject/selinux/pull/71

Post by Jan Zarsky
Function dbase_llist_iterate iterates over records and checks return
value of iterate function. According to a manpage semanage_iterate(3),
handler can return value 1 for early exit. dbase_llist_iterate
currently checks for return value > 1, which does not include
expected value 1. This affects most of the semanage_*_iterate
and semanage_*_local functions.
---
libsemanage/src/database_llist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libsemanage/src/database_llist.c b/libsemanage/src/database_llist.c
index 8ce2e2c1..c8f4ff0b 100644
--- a/libsemanage/src/database_llist.c
+++ b/libsemanage/src/database_llist.c
@@ -263,7 +263,7 @@ int dbase_llist_iterate(semanage_handle_t * handle,
if (rc < 0)
goto err;
- else if (rc > 1)
+ else if (rc > 0)
break;
}
--
2.14.3

--
Respectfully,

William C Roberts