[SSSD] [PATCH] failover: Protect against empty host names
Jakub Hrozek
jhrozek at redhat.com
Wed Jan 2 16:52:57 UTC 2013
On Wed, Jan 02, 2013 at 12:08:31PM +0100, Pavel Březina wrote:
> On 01/02/2013 12:07 PM, Pavel Březina wrote:
> >On 12/03/2012 04:11 PM, Pavel Březina wrote:
> >>On 12/03/2012 03:02 PM, Michal Židek wrote:
> >>>On 12/03/2012 01:35 PM, Pavel Březina wrote:
> >>>>On 11/30/2012 01:32 PM, Michal Židek wrote:
> >>>>>On 11/30/2012 11:35 AM, Pavel Březina wrote:
> >>>>>>On 11/29/2012 05:08 PM, Michal Židek wrote:
> >>>>>>>On 11/27/2012 03:18 PM, Pavel Březina wrote:
> >>>>>>>>On 11/26/2012 02:40 PM, Michal Židek wrote:
> >>>>>>>>>On 11/23/2012 10:11 AM, Pavel Březina wrote:
> >>>>>>>>>>On 10/17/2012 04:01 PM, Michal Židek wrote:
> >>>>>>>>>>>On 10/16/2012 11:36 AM, Pavel Březina wrote:
> >>>>>>>>>>>>On 10/15/2012 12:30 PM, Michal Židek wrote:
> >>>>>>>>>>>>>Added new parameter to split_on_separator that allows to skip
> >>>>>>>>>>>>>empty values.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>https://fedorahosted.org/sssd/ticket/1484
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>Patch is in attachment.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>Michal
> >>>>>>>>>>>>
> >>>>>>>>>>>>Nack. See below.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>--- a/src/util/util.c
> >>>>>>>>>>>>>+++ b/src/util/util.c
> >>>>>>>>>>>>>@@ -31,7 +31,8 @@
> >>>>>>>>>>>>> * the separator is a string, and is case-sensitive.
> >>>>>>>>>>>>> * optionally single values can be trimmed of of spaces and
> >>>>>>>>>>>>>tabs */
> >>>>>>>>>>>>> int split_on_separator(TALLOC_CTX *mem_ctx, const char *str,
> >>>>>>>>>>>>>- const char sep, bool trim, char
> >>>>>>>>>>>>>***_list,
> >>>>>>>>>>>>>int
> >>>>>>>>>>>>>*size)
> >>>>>>>>>>>>>+ const char sep, bool trim, bool
> >>>>>>>>>>>>>skip_empty,
> >>>>>>>>>>>>>+ char ***_list, int *size)
> >>>>>>>>>>>>> {
> >>>>>>>>>>>>> const char *t, *p, *n;
> >>>>>>>>>>>>> size_t l, len;
> >>>>>>>>>>>>>@@ -80,6 +81,11 @@ int split_on_separator(TALLOC_CTX *mem_ctx,
> >>>>>>>>>>>>>const
> >>>>>>>>>>>>>char *str,
> >>>>>>>>>>>>> }
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> if (len == 0) {
> >>>>>>>>>>>>>+ if (skip_empty) {
> >>>>>>>>>>>>>+ /* Move to next string and continue */
> >>>>>>>>>>>>>+ t = n;
> >>>>>>>>>>>>>+ continue;
> >>>>>>>>>>>>>+ }
> >>>>>>>>>>>>
> >>>>>>>>>>>>Move this^ condition before talloc_realloc.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>I did not like the function at all, it seemed to be written for
> >>>>>>>>>>>different purpose but slightly modified to fit our needs. I
> >>>>>>>>>>>wrote a
> >>>>>>>>>>>new
> >>>>>>>>>>>version in this patch and added test for it.
> >>>>>>>>>>>
> >>>>>>>>>>>New patch attached.
> >>>>>>>>>>>
> >>>>>>>>>>>Michal
> >>>>>>>>>>
> >>>>>>>>>>Hi,
> >>>>>>>>>>can you rebase it atop the current master please?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>Sure, here is the new patch.
> >>>>>>>>>
> >>>>>>>>>Thanks
> >>>>>>>>>Michal
> >>>>>>>>
> >>>>>>>>Thanks.
> >>>>>>>>
> >>>>>>>>Nack.
> >>>>>>>>
> >>>>>>>>Test issues:
> >>>>>>>>
> >>>>>>>>>+ {
> >>>>>>>>>+ ", ,,",
> >>>>>>>>>+ (const char*[]){NULL,},
> >>>>>>>>>+ true, true,
> >>>>>>>>>+ 0, 0
> >>>>>>>>>+ },
> >>>>>>>>
> >>>>>>>>What is the meaning of the comma after NULL?
> >>>>>>>>
> >>>>>>>
> >>>>>>>It had no meaning. Deleted.
> >>>>>>>
> >>>>>>>>>+ for (i = 0; str_ref = sts[a].expected_list[i], str_out =
> >>>>>>>>>list[i]; i++) {
> >>>>>>>>>+ fail_unless(!strcmp(str_ref, str_out),
> >>>>>>>>>+ "Expected:%s Got:%s\n", str_ref,
> >>>>>>>>>str_out);
> >>>>>>>>>+ }
> >>>>>>>>
> >>>>>>>>Please use strcmp() == 0.
> >>>>>>>>
> >>>>>>>>Function issues:
> >>>>>>>>
> >>>>>>>>> char **r;
> >>>>>>>>
> >>>>>>>>Please, choose a better name for this variable. Something that
> >>>>>>>>explains
> >>>>>>>>its meaning. Maybe 'reallocated_list'?
> >>>>>>>
> >>>>>>>Done.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> /* Trim leading whitespace if needed */
> >>>>>>>>> while (trim && isspace(*substr_begin) &&
> >>>>>>>>> substr_begin < substr_end) {
> >>>>>>>>> substr_begin++;
> >>>>>>>>> substr_len--;
> >>>>>>>>> }
> >>>>>>>>>
> >>>>>>>>> /* Trim trailing whitespace if needed */
> >>>>>>>>> while (trim && substr_end - 1 > substr_begin &&
> >>>>>>>>> isspace(*(substr_end-1))) {
> >>>>>>>>> substr_end--;
> >>>>>>>>> substr_len--;
> >>>>>>>>> }
> >>>>>>>>
> >>>>>>>>Put the condition on one line. And separate trim from while please.
> >>>>>>>>if (trim) {
> >>>>>>>> while () // leading
> >>>>>>>>
> >>>>>>>> while () // trailing
> >>>>>>>>}
> >>>>>>>>
> >>>>>>>>> if (!(skip_empty && substr_len == 0)) {
> >>>>>>>>
> >>>>>>>>if (!skip_empty || substr_len == 0) is more readable in my opinion.
> >>>>>>>>
> >>>>>>>
> >>>>>>>Done.
> >>>>>>>
> >>>>>>>>> if (substr_len == 0) {
> >>>>>>>>> list[num_strings] = talloc_strdup(list, "");
> >>>>>>>> ^
> >>>>>>>>> } else {
> >>>>>>>>> list[num_strings] = talloc_strndup(list,
> >>>>>>>>>substr_begin,
> >>>>>>>>> substr_len);
> >>>>>>>> ^
> >>>>>>>>> }
> >>>>>>>>
> >>>>>>>>Indentation...
> >>>>>>>
> >>>>>>>Done.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> talloc_free(list);
> >>>>>>>>> return ENOMEM;
> >>>>>>>>
> >>>>>>>>Can you use "fail" pattern instead of calling the former each time
> >>>>>>>>allocation fails?
> >>>>>>>
> >>>>>>>Label "done:" added.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>Otherwise it looks good. I like the function.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>New patch attached.
> >>>>>>>
> >>>>>>>Thanks
> >>>>>>>Michal
> >>>>>>
> >>>>>>Hi,
> >>>>>>I have few more nitpicks.
> >>>>>>
> >>>>>>In tests:
> >>>>>>> {
> >>>>>>> NULL,
> >>>>>>> NULL,
> >>>>>>> false,false,
> >>>>>> ^ missing comma :P
> >>>>>>> 0, EINVAL
> >>>>>>> },
> >>>>>
> >>>>>I can see only missing space :P
> >>>>>
> >>>>>>
> >>>>>>Function:
> >>>>>>> while (isspace(*substr_begin) &&
> >>>>>>> substr_begin < substr_end) {
> >>>>>>> substr_begin++;
> >>>>>>> substr_len--;
> >>>>>>> }
> >>>>>>>
> >>>>>>> /* Trim trailing whitespace */
> >>>>>>> while (substr_end - 1 > substr_begin &&
> >>>>>>> isspace(*(substr_end-1))) {
> >>>>>>> substr_end--;
> >>>>>>> substr_len--;
> >>>>>>> }
> >>>>>>
> >>>>>>Can you put those conditions on one line? Both fit into 80 char limit
> >>>>>>perfectly and separating them doesn't really help clarity.
> >>>>>>
> >>>>>>> reallocated_list = talloc_realloc(mem_ctx, list, char*,
> >>>>>>> num_strings + 2);
> >>>>>> ^ indentation :)
> >>>>>>and
> >>>>>>> list = talloc(mem_ctx, char *);
> >>>>>>
> >>>>>>Allocate first on NULL and then talloc_steal() it to mem_ctx. This
> >>>>>>way
> >>>>>>we can catch memory leak. (I know it's obvious there are not any at
> >>>>>>the
> >>>>>>moment, but still...)
> >>>>>>
> >>>>>>And last comment - since the function was completely rewritten, can
> >>>>>>you
> >>>>>>pick either ptr == NULL or !ptr so it is consistent?
> >>>>>>
> >>>>>>I'm sorry, I should have came with these earlier.
> >>>>>>
> >>>>>>Thanks.
> >>>>>>
> >>>>>
> >>>>>New patch attached.
> >>>>>
> >>>>>Thanks
> >>>>>Michal
> >>>>
> >>>>Hi,
> >>>>we're almost there, but still nack :(
> >>>>
> >>>>tmp_ctx is always NULL this way, so it doesn't work as a talloc context
> >>>>at all. You could just use:
> >>>>
> >>>>talloc_realloc(NULL, list, ...);
> >>>>talloc_free(list);
> >>>>
> >>>>or initialize tmp_ctx with talloc_new(NULL).
> >>>
> >>>Yes, that is what I thought I did :-D . I see I didn't.
> >>>
> >>>>
> >>>>But I like it better with tmp context after all, because you don't have
> >>>>to use reallocated_list then and the code is more straightforward.
> >>>>
> >>>>So you can use:
> >>>> TALLOC_CTX *tmp_ctx = NULL;
> >>>>
> >>>> tmp_ctx = talloc_new(NULL);
> >>>> if (tmp_ctx == NULL) {...}
> >>>>
> >>>> ...
> >>>> list = talloc_realloc(tmp_ctx, list, char*, num_strings + 2);
> >>>> if (list == NULL) {
> >>>> ret = ENOMEM;
> >>>> goto done;
> >>>> }
> >>>> ...
> >>>>done:
> >>>> talloc_free(tmp_ctx)
> >>>
> >>>You are right, with the tmp context, the reallocated_list is not needed.
> >>>
> >>>
> >>>New patch attached.
> >>>
> >>>Thanks
> >>>Michal
> >>
> >>Hi,
> >>I dared to amend your patch with following changes:
> >>
> >>- if (substr_len == 0) {
> >>- list[num_strings] = talloc_strdup(list, "");
> >>- } else {
> >>- list[num_strings] = talloc_strndup(list, substr_begin,
> >>- substr_len);
> >>- }
> >>+ /* empty string is stored for substr_len == 0 */
> >>+ list[num_strings] = talloc_strndup(list, substr_begin,
> >>substr_len);
> >> if (list[num_strings] == NULL) {
> >> ret = ENOMEM;
> >> goto done;
> >>
> >>talloc_strndup(list, substr_begin, 0) == talloc_strdup(list, "")
> >>so we don't need to branch here
> >>
> >> -107,7 +102,7 @@ int split_on_separator(TALLOC_CTX *mem_ctx, const
> >>char *str,
> >> /* No allocations were done, make space for the NULL */
> >> list = talloc(tmp_ctx, char *);
> >> if (list == NULL) {
> >>- ret ENOMEM;
> >>+ ret = ENOMEM;
> >> goto done;
> >> }
> >> }
> >>
> >>You've somehow managed to delete = which was present in the previous
> >>iteration.
> >>
> >>I tried this patch with various weird ldap uris and it works splendidly.
> >>
> >>ACK from me
> >
> >Just a remainder. This patch has been waiting for some time.
>
> s/remainder/reminder :-)
I agree with how you amended Michal's patch. I haven't reviewed the
patch fully, though, I trust your good judgement there :-)
Pushed to master.
More information about the sssd-devel
mailing list