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

Lukas Slebodnik lslebodn at redhat.com
Mon Apr 7 19:43:19 UTC 2014


On (07/04/14 14:20), 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.
If we want to be secure; we should:
    1. allocate array with length "data_len + 1"
    2. call memcopy
    3. terminate our internal_array with NUL '\0'
Because there is still possibility that data_buf needn't be null-terminated
even if it is written in documentation.

But it will be the internal change and API wil be the same.

>The caller however knows the size of the buffer so it is easier for
>it to get the length in as 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 am OK of exposing both though I think the second one is extra work
>with not much of a value.
>Feel free to file an RFE to add the second function as a wrapper.
No it isn't necessary.

yelley explained his use case in another mail
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-April/019023.html

LS



More information about the sssd-devel mailing list