[SSSD] [ding-libs] Patches for #2267, build issue and tight loop

Yassir Elley yelley at redhat.com
Mon Apr 7 18:51:23 UTC 2014



----- Original Message -----
> On 04/07/2014 02:20 PM, Dmitri Pal wrote:
> > On 04/07/2014 05:47 AM, Lukas Slebodnik wrote:
> >> On (04/04/14 17:47), Dmitri Pal wrote:
> >>>>>>> Just a couple minor edits to the language of the doxygen:
> >>>>>>>
> >>>>>>>
> >>>>>>> diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h
> >>>>>>> index 9a4eb58..4ef98dc 100644
> >>>>>>> - --- a/ini/ini_configobj.h
> >>>>>>> +++ b/ini/ini_configobj.h
> >>>>>>> @@ -483,6 +483,31 @@ int ini_config_file_open(const char *filename,
> >>>>>>>                            struct ini_cfgfile **file_ctx);
> >>>>>>>
> >>>>>>>   /**
> >>>>>>> + * @brief Create a configuration file object using memory buffer.
> >>>>>>>
> >>>>>>> @brief Create a configuration file object from a NULL-terminated
> >>>>>>> buffer.
> >>>>>>>
> >>>>>>> + *
> >>>>>>> + * Create a file object for parsing a configuration file.
> >>>>>>> + * Configuration will use provided memory instead of the actual
> >>>>>>> file.
> >>>>>>>
> >>>>>>> Configuration will use the provided memory buffer instead of a
> >>>>>>> real file.
> >>>>>>>
> >>>>>>> + *
> >>>>>>> + * A "configuration file object" is different from
> >>>>>>> + * a "configuration object". The former stores metadata
> >>>>>>> + * about a file the configuration data is read from,
> >>>>>>>
> >>>>>>> about a file from which the configuration data is read,
> >>>>>>> (this is a very esoteric bit of English)
> >>>>>>>
> >>>>>>> + * while the latter holds the configuration itself.
> >>>>>>>
> >>>>>>> while the latter holds the configuration data itself.
> >>> I split it into a separate ticket.
> >>> https://fedorahosted.org/sssd/ticket/2307
> >>> I suspect that there are other places like this that require some
> >>> polish.
> >>> Somone needs to review doxygen comments and ding-libs and make them
> >>> use better English.
> >>>
> >>>
> >>>> + *
> >>>> + * @param[in]  data_buf         In memory configuration data.
> >>>>
> >>>> In-memory
> >>>>
> >>>> + *                              Needs to be NULL terminated.
> >>>>
> >>>> NULL-terminated
> >>>>
> >>>> + * @param[in]  data_len         Length of memory data
> >>>> + *                              not including terminating NULL.
> >>>> + * @param[out] file_ctx         Configuration file object.
> >>>> + *
> >>>> + * @return 0 - Success.
> >>>> + * @return EINVAL - Invalid parameter.
> >>>> + * @return ENOMEM - No memory.
> >>>> + */
> >>>> +int ini_config_file_from_mem(void *data_buf,
> >>>> +                             uint32_t data_len,
> >>>> +                             struct ini_cfgfile **file_ctx);
> >>>> +
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> One additional question: is it expected that data_buf will contain NUL
> >>>> values in it other than the terminator? If not, why does data_len
> >>>> exist?
> >>> Interesting question.
> >>> Here is how I see it.
> >>>
> >>> Man page for fmemopen says:
> >>>
> >>>        FILE *fmemopen(void *buf, size_t size, const char *mode);
> >>>
> >>>       ...
> >>>
> >>> DESCRIPTION
> >>>        The  fmemopen()  function opens a stream that permits the
> >>> access speci-
> >>>        fied by mode.  The stream allows I/O to be performed on the
> >>> string  or
> >>>        memory  buffer  pointed  to  by buf.  This buffer must be at
> >>> least size
> >>>        bytes long.
> >>>
> >>>
> >>> At least!
> >>> But in reality if it is of the same value the function does not work.
> >>> I checked it twice and valgrind confirmed that it is trying to read
> >>> past the end of the buffer.
> >>>
> >>> So the NULL termination is the attempt to work around the bug in
> >>> fmemopen.
> >>> The caller of the function needs to make sure that the function would
> >>> work thus we say to NULL terminate the buffer.
> >>>
> >>> I think I need to add another test. This test would use part of the
> >>> buffer not the whole one.
> >>> https://fedorahosted.org/sssd/ticket/2306
> >>>
> >>>
> >>> Can we push the patches as the thread gets long and hard to deal with.
> >>> All comments are either addressed or tracked as separate tickets.
> >>>
> >> I am fine with current version but I have another idea.
> >> (I know it is little bit late but it is better to ask before pushing
> >> patches to
> >> public repo)
> >>
> >> Could we make next function internal:
> >> int ini_config_file_from_mem(void *data_buf,
> >>                               uint32_t data_len,
> >>                               struct ini_cfgfile **file_ctx);
> >>
> >> and public function will be
> >> int ini_config_file_from_string(const char *string_config
> >>                                  struct ini_cfgfile **file_ctx);
> >>
> >> I do not know requirements from yelley; so my idea needn't be
> >> acceptable.
> >>
> >> LS
> > I prefer not to for a reason. This function would require to call
> > strlen() to find the end of the buffer.
> > We do not know anything about the caller or how he allocated buffer so
> > we can't call strnlen() which is safer.
> > The caller however knows the size of the buffer so it is easier for it
> > to get the length in as safe way.
> ... in _a_ safe way ...
> 
> > In many case when you read data from a socket of file you already know
> > the size of the data so you do not need to count bytes and thus can
> > save cycles.
> >

I need to be able to use an array of bytes (containing arbitrary binary data) to initialize the ini_config object. Since this is binary data, the ini_config_file_from_string function would not be adequate to handle my use case (since the NUL character doesn't signify termination for an arbitrary array of bytes).

Regards,
Yassir.



More information about the sssd-devel mailing list