[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data jhrozek commented: """ * master: a40c6b4280f319efb935a9c9d3b83486a0f4d2d3 """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-377324396 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data jhrozek commented: """ OK, let's have this patch merged /as a stopgap/ so that we can have a fix in fedora and work on a better fix in the meantime without waiting for the perfect patch and having crashes in the meantime. """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-377235952 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data fidencio commented: """ @jhrozek, @pbrezina, Without looking at the code I'd give a +1 to Pavel's suggestion as well. However, taking a look at the code makes me quite reluctant to go for it. Let me try to explain my reasons and then I'll just wait to be convinced the opposite (and here I'm hoping it'll happen): - I do **not** know what kind of data is being passed to tcurl_http_send(): Without knowing exactly what has been passed to tcurl_http_send(), I sincerely have no clue on how to determine whether this data contains or not a '\0' in the end. Without knowing it, it's really hard to know when and where to make use of `sss_iobuf_get_data_as_string()`. I hope that you guys have a better clue about these details and will help me to have a better idea as well (a sheet with all the data we receive and whether those are NULL or non-NULL terminated would be what I'm looking for here). - The current code adds NULL terminated strings to `sss_iobuf`: Shall these change as well? Because if we're adding a new method to explicitly treat bytes as bytes and strings as strings ... we should start by **only** adding bytes content to sss_iobuf. I do believe this has been happening because the data passed to the write_callback of curl is explicitly **non** NULL terminated, as explicitly said here: https://curl.haxx.se/libcurl/c/CURLOPT_WRITEFUNCTION.html - Is all those changes worth it considering https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org/message/YM5JLYLOJ7KRE5OXSISS5UOGULOZAREP/): I'm asking this because the suggestion given, although it looks simple, would take me a quite long time to implement (which may not be the case for someone who's more used to this code) ... and it sincerely feels like we'd be adding a new method and changing a lot of things to conform with one specific place. So, please, help me to understand better your ideas and then I'll get back to this. In any case, I'd really like to suggest: - Let's have this patch merged and have the fixes backported all over the place; - Let's open a ticket for the changes (kudos if the questions raised here are all answered there); """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375789900 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data jhrozek commented: """ +1 to the suggestion of adding sss_iobuf_as_string which would add the NULL terminator and duplicate (or even at that point just return, I frankly don't see the need to duplicate) the dp pointer as a string """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375275294 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data pbrezina commented: """ Simple fix: given the use cases, we can make `tcurl_http_recv` to ensure that data are NULL-terminated. Technically correct fix: `sss_iobuf_get_data` which is used here returns binary data, not a string. So it is perfectly fine to return non NULL-terminated data here. There is `sss_iobuf_read_stringz` which should be used instead to read string, but it won't help if the data is not already NULL-terminated. So there should be a way (new iobuf function possibly) to return `strndup(buf, len)`. BTW, current implementation of `sss_iobuf_read_stringz` is not very safe as we can simple access data outside our buffer in `memchr`. """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375266315 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data jhrozek commented: """ Dunno, I keep going back and forth on this myself.. Let's table this and talk on IRC tomorrow :) """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375064108 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data fidencio commented: """ That's another approach that can be taken. If you prefer taking this path, let me go for this. """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375059176 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data jhrozek commented: """ Umm looking at the code more, shouldn't we rather make sure that whatever we get out of tcurl is NULL-terminated? Wouldn't that be a safer way also for the future, I'm sure that otherwise someone will "optimize" the code in a couple of years again.. """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375058373 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data fidencio commented: """ Right, I'll do a complete check on that. I've already mentioned that it has to be checked in the issue I've opened ... About the json_loads(), the other two places do we also get non-null strings? Or it's something you don't know from the top of your head? """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375057798 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data jhrozek commented: """ Actually, there is more. I think I was writing the code under the impression that you will get a NULL-teminated string in the secrets output. But I would suggest to check also all calls to `sss_iobuf_get_data`. """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375057313 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
[SSSD] [sssd PR#542][comment] KCM: Use json_loadb() when dealing with sss_iobuf data
URL: https://github.com/SSSD/sssd/pull/542 Title: #542: KCM: Use json_loadb() when dealing with sss_iobuf data jhrozek commented: """ There is another usage of `json_loads` in `sec_value_to_json`, do you think it would make sense to fix that one as well? Otherwise LGTM, very nice catch, thank you. """ See the full comment at https://github.com/SSSD/sssd/pull/542#issuecomment-375055443 ___ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
