Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
Hi, sure.
You're leaking memory in ifp_bus_path_escape/ifp_bus_path_unescape when the append operation fails, but I think we can keep it that way since the mem_ctx is bounded to a request which is supposed to end shortly.
Cleaner solution would be to free the path on error, but I'll keep it up to yours decision.
Ack from my side.
On Sun, 2014-05-11 at 19:18 +0200, Pavel Březina wrote:
On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
Hi, sure.
You're leaking memory in ifp_bus_path_escape/ifp_bus_path_unescape when the append operation fails, but I think we can keep it that way since the mem_ctx is bounded to a request which is supposed to end shortly.
Cleaner solution would be to free the path on error, but I'll keep it up to yours decision.
Ack from my side.
I think it would be nicer to to free the buffer on error, that would also allow us to use leak checks in the unit tests.
I'll send new versions.
On Sun, May 11, 2014 at 10:58:24PM +0200, Jakub Hrozek wrote:
On Sun, 2014-05-11 at 19:18 +0200, Pavel Březina wrote:
On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
Hi, sure.
You're leaking memory in ifp_bus_path_escape/ifp_bus_path_unescape when the append operation fails, but I think we can keep it that way since the mem_ctx is bounded to a request which is supposed to end shortly.
Cleaner solution would be to free the path on error, but I'll keep it up to yours decision.
Ack from my side.
I think it would be nicer to to free the buffer on error, that would also allow us to use leak checks in the unit tests.
I'll send new versions.
Thanks for the review, new patches are attached.
On 05/12/2014 05:09 PM, Jakub Hrozek wrote:
On Sun, May 11, 2014 at 10:58:24PM +0200, Jakub Hrozek wrote:
On Sun, 2014-05-11 at 19:18 +0200, Pavel Březina wrote:
On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
Hi, sure.
You're leaking memory in ifp_bus_path_escape/ifp_bus_path_unescape when the append operation fails, but I think we can keep it that way since the mem_ctx is bounded to a request which is supposed to end shortly.
Cleaner solution would be to free the path on error, but I'll keep it up to yours decision.
Ack from my side.
I think it would be nicer to to free the buffer on error, that would also allow us to use leak checks in the unit tests.
I'll send new versions.
Thanks for the review, new patches are attached.
Ack.
On (12/05/14 17:09), Jakub Hrozek wrote:
On Sun, May 11, 2014 at 10:58:24PM +0200, Jakub Hrozek wrote:
On Sun, 2014-05-11 at 19:18 +0200, Pavel Březina wrote:
On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
Hi, sure.
You're leaking memory in ifp_bus_path_escape/ifp_bus_path_unescape when the append operation fails, but I think we can keep it that way since the mem_ctx is bounded to a request which is supposed to end shortly.
Cleaner solution would be to free the path on error, but I'll keep it up to yours decision.
Ack from my side.
I think it would be nicer to to free the buffer on error, that would also allow us to use leak checks in the unit tests.
I'll send new versions.
Thanks for the review, new patches are attached.
From 428c7e9febf470ca737ab93e2cce3b20ee4f482b Mon Sep 17 00:00:00 2001 From d06ba377f0cba19982dc3ce1d192cdb4919662a0 Mon Sep 17 00:00:00 2001 From: Pavel Březina pbrezina@redhat.com Date: Sun, 11 May 2014 16:18:06 +0200 Subject: [PATCH 2/3] IFP: Add a utility function to reply with an object path
src/responder/ifp/ifp_private.h | 6 ++++++ src/responder/ifp/ifpsrv_util.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
diff --git a/src/responder/ifp/ifp_private.h b/src/responder/ifp/ifp_private.h index 286cc75c9b774b0fd9d18ef6166a7c3f1ae41e53..580be9abd8923c834250c7882af0512ae6490003 100644 --- a/src/responder/ifp/ifp_private.h +++ b/src/responder/ifp/ifp_private.h @@ -73,6 +73,12 @@ const char *ifp_path_strip_prefix(const char *path, const char *prefix); char *ifp_bus_path_unescape(TALLOC_CTX *mem_ctx, const char *path); char *ifp_bus_path_escape(TALLOC_CTX *mem_ctx, const char *path);
+char *_ifp_reply_objpath(TALLOC_CTX *mem_ctx, const char *base,
const char *part, ...);+#define ifp_reply_objpath(mem_ctx, base, ...) \
- _ifp_reply_objpath(mem_ctx, base, ##__VA_ARGS__, NULL)
errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict, struct ldb_message_element *el); const char **ifp_parse_attr_list(TALLOC_CTX *mem_ctx, const char *conf_str); diff --git a/src/responder/ifp/ifpsrv_util.c b/src/responder/ifp/ifpsrv_util.c index 119e0d277664905a462c104abb6305258af2acca..b7094560fc2f00ee407e7bf78faac82b7c3b91ee 100644 --- a/src/responder/ifp/ifpsrv_util.c +++ b/src/responder/ifp/ifpsrv_util.c @@ -203,6 +203,47 @@ char *ifp_bus_path_unescape(TALLOC_CTX *mem_ctx, const char *path) return safe_path; }
+char * +_ifp_reply_objpath(TALLOC_CTX *mem_ctx, const char *base,
const char *part, ...)+{
- char *safe_part;
- char *path = NULL;
- va_list va;
- if (base == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "Wrong object path base!\n");return NULL;- }
- path = talloc_strdup(mem_ctx, base);
- if (path == NULL) return NULL;
- va_start(va, part);
^^^^^^^^^^^^^^^^^^ start
- while (part != NULL) {
safe_part = ifp_bus_path_escape(mem_ctx, part);if (safe_part == NULL) {DEBUG(SSSDBG_OP_FAILURE, "Could not add [%s] to objpath\n", part);goto fail;
^^^^^^^^^^ in case of failure va_end is not called.
}path = talloc_asprintf_append(path, "/%s", safe_part);talloc_free(safe_part);if (path == NULL) {goto fail;
^^^^^^^^^^ in case of failure va_end is not called.
}part = va_arg(va, const char *);- }
- va_end(va);
- return path;
+fail:
- talloc_free(path);
- return NULL;
+}
errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict, struct ldb_message_element *el) { -- 1.9.0
reported by coverity
LS
On 12.05.2014 17:09, Jakub Hrozek wrote:
On Sun, May 11, 2014 at 10:58:24PM +0200, Jakub Hrozek wrote:
On Sun, 2014-05-11 at 19:18 +0200, Pavel Březina wrote:
On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
This is a really ugly part of DBus :S
I wish object paths weren't so restrictive. There's no real reason for it, IMO. But oh well, this is what we have to live with.
/* D-Bus spec says:* ** * Each element must only contain the ASCII characters* "[A-Z][a-z][0-9]_"* */if ((c >= 'A' && c <= 'Z')|| (c >= 'a' && c <= 'z')|| (c >= '0' && c <= '9')|| c == '_') {safe_path = talloc_asprintf_append_buffer(safe_path, "%c", c);if (safe_path == NULL) {goto done;}
If you're expecting to unescape these paths, then you have to escape the underscore as well, and not let it through verbatim.
- /* Special case for the empty string */
- if (strcmp(path, "_") == 0) {
I may be confused, but I don't see similar logic for empty strings in ifp_bus_path_escape().
Stef
On Tue, May 13, 2014 at 06:57:28PM +0200, Stef Walter wrote:
On 12.05.2014 17:09, Jakub Hrozek wrote:
On Sun, May 11, 2014 at 10:58:24PM +0200, Jakub Hrozek wrote:
On Sun, 2014-05-11 at 19:18 +0200, Pavel Březina wrote:
On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
This is a really ugly part of DBus :S
I wish object paths weren't so restrictive. There's no real reason for it, IMO. But oh well, this is what we have to live with.
/* D-Bus spec says:* ** * Each element must only contain the ASCII characters* "[A-Z][a-z][0-9]_"* */if ((c >= 'A' && c <= 'Z')|| (c >= 'a' && c <= 'z')|| (c >= '0' && c <= '9')|| c == '_') {safe_path = talloc_asprintf_append_buffer(safe_path, "%c", c);if (safe_path == NULL) {goto done;}If you're expecting to unescape these paths, then you have to escape the underscore as well, and not let it through verbatim.
Ah, correct, thanks!
- /* Special case for the empty string */
- if (strcmp(path, "_") == 0) {
I may be confused, but I don't see similar logic for empty strings in ifp_bus_path_escape().
That's also a bug.
I added unit checks for both these scenarios to the test_path_escape_unescape() unit test.
Stef
Thanks for the review!
new patches are attached.
On (15/05/14 20:32), Jakub Hrozek wrote:
On Tue, May 13, 2014 at 06:57:28PM +0200, Stef Walter wrote:
On 12.05.2014 17:09, Jakub Hrozek wrote:
On Sun, May 11, 2014 at 10:58:24PM +0200, Jakub Hrozek wrote:
On Sun, 2014-05-11 at 19:18 +0200, Pavel Březina wrote:
On 05/11/2014 04:40 PM, Jakub Hrozek wrote:
Hi,
the attached patches add utility functions that allow the InfoPipe responder to reply with an object path, escaped if needed.
The ifp_reply_objpath() function was initially in my tree, but Pavel improved it quite a bit and fixed some bugs, so the patch should be attributed to him. I'm also fine with his code...Pavel, can you ack^Wreview the other two?
This is a really ugly part of DBus :S
I wish object paths weren't so restrictive. There's no real reason for it, IMO. But oh well, this is what we have to live with.
/* D-Bus spec says:* ** * Each element must only contain the ASCII characters* "[A-Z][a-z][0-9]_"* */if ((c >= 'A' && c <= 'Z')|| (c >= 'a' && c <= 'z')|| (c >= '0' && c <= '9')|| c == '_') {safe_path = talloc_asprintf_append_buffer(safe_path, "%c", c);if (safe_path == NULL) {goto done;}If you're expecting to unescape these paths, then you have to escape the underscore as well, and not let it through verbatim.
Ah, correct, thanks!
- /* Special case for the empty string */
- if (strcmp(path, "_") == 0) {
I may be confused, but I don't see similar logic for empty strings in ifp_bus_path_escape().
That's also a bug.
I added unit checks for both these scenarios to the test_path_escape_unescape() unit test.
Stef
Thanks for the review!
new patches are attached.
You forgot to fix coverity issue https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019503.html
LS
On Thu, May 15, 2014 at 08:54:53PM +0200, Lukas Slebodnik wrote:
You forgot to fix coverity issue https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019503.html
LS
I did, thanks.
On Thu, May 15, 2014 at 09:04:34PM +0200, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 08:54:53PM +0200, Lukas Slebodnik wrote:
You forgot to fix coverity issue https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019503.html
LS
I did, thanks.
Bump. I'd like to get these patches to 1.12 beta, any other review opinions?
On 21.05.2014 22:46, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:04:34PM +0200, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 08:54:53PM +0200, Lukas Slebodnik wrote:
You forgot to fix coverity issue https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019503.html
LS
I did, thanks.
Bump. I'd like to get these patches to 1.12 beta, any other review opinions?
ACK
Stef
On Thu, May 22, 2014 at 12:49:27PM +0200, Stef Walter wrote:
On 21.05.2014 22:46, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:04:34PM +0200, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 08:54:53PM +0200, Lukas Slebodnik wrote:
You forgot to fix coverity issue https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019503.html
LS
I did, thanks.
Bump. I'd like to get these patches to 1.12 beta, any other review opinions?
ACK
Stef
Thank you for the review, pushed to master: e22d84a534c0b093a31f8473e6c3cbabdb5f5c82 78f2f0bbb999d40782f8beb940f79bc522cdcb2d 1a546ae8250c6dbcff0ce775fc811529e02c360c
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Stef Walter