Let iniparser_getstring() not return NULL if there is a section (not key) with specified name

[dofuuz]

iniparser_getboolean() can segfault on malformed config files. #125
There are similar problem in iniparser_getlongint() and iniparser_getdouble().

iniparser_getstring() returns NULL if there are section(not key) with that name.

The key does not exists, so it's better to return default fallback.
And returning NULL for "string" is not likely to be considered by programmers, which can be security problem.

Fixes #125

[uncleura]

Returning "default" instead on NULL in the case of "section" breaks completely functions iniparser_dumpsection_ini() and iniparser_dump_ini() because iniparser_find_entry() will no longer work for sections (line 303 of iniparser.c). Possibly the best we can do is to return "another_one_default" from iniparser_getstring() function in the case of section.

[lmoellendorf]

@dofuuz Please have a look at #125 (comment)

[dofuuz]

@lmoellendorf
The segfault problem at #125 was fixed with later PR #146(merged).

What this PR is different from #146 is, iniparser_getstring() won't return NULL even if the value is NULL(caused by mallformed ini, or by explicitly set with iniparser_set()). Instead of NULL, it will return string "(null)"

In my opinion, returning NULL can be potential problem, so returning "(null)" string is more safe option.

[lmoellendorf]

In my opinion, returning NULL can be potential problem, so returning "(null)" string is more safe option.

Maybe I am missing something, but I do not understand why parsing a returned pointer for "(null)" should be safer than a quick check for NULL.

As far as I know all standard functions which return pointers, return NULL on error. In my opinion checking returned or passed pointers for NULL is a mandatory thing to do.

[lmoellendorf]

@dofuuz Thank you for the clarification.

[dofuuz]

@lmoellendorf
Please reconsider merging this PR.

Even authors of iniparser missed the null check and made CVE entry (CVE-2023-33461). #125 #144
Users are more likely to miss that too. 😧

Making an API Hard to Misuse is good principle of making a library.

[lmoellendorf]

Even authors of iniparser missed the null check and made CVE entry (CVE-2023-33461).

The CVE reads [emphasizes by me]:

iniparser v4.1 is vulnerable to NULL Pointer Dereference in function iniparser_getlongint which misses check NULL for function iniparser_getstring's return.

This was in internal bug, which is fixed.

#125 #144 Users are more likely to miss that too. 😧

I get your argumentation. Programmers tend to forget parameter and return value validation.

But I would rather not resort to returning an arbitrary string instead of NULL.

Making an API Hard to Misuse is good principle of making a library.

If so, I would rather suggest an API like this:

/*-------------------------------------------------------------------------*/
/**
  @brief    Get the string associated to a key
  @param[out] val   Pointer to statically allocated character string
  @param      d     Dictionary to search
  @param      key   Key string to look for
  @param      def   Default value to return if key not found.
  @return     0 on success, -1 else

  This function queries a dictionary for a key. A key as read from an
  ini file is given as "section:key". If the key cannot be found,
  the pointer passed as 'def' is returned.
  The returned char pointer is pointing to a string allocated in
  the dictionary, do not free or modify it.
 */
/*--------------------------------------------------------------------------*/
int iniparser_getstring(const char **val, const dictionary * d, const char * key, const char * def);

...or I would return a pointer to a statically allocated character string, denoting an empty string like it is done in protobuf-c. Making this string part of the API allows for a simple pointer check as used to with NULL.

This will have to be discussed for v5.x.

[dofuuz]

Returning "(null)" is not so wrong. With common compilers (including gcc, msvc),

printf("%s", NULL);

prints (null) instead of crashing entire program. (Its behavior is undefined in C standard)

So here is my sugesstion (it's modified from the latter option of your previous comment)

extern const char iniparser_null_string[];
/*-------------------------------------------------------------------------*/
/**
  @brief    Get the string associated to a key
  @param    d       Dictionary to search
  @param    key     Key string to look for
  @param    def     Default value to return if key not found.
  @return   pointer to statically allocated character string

  This function queries a dictionary for a key. A key as read from an
  ini file is given as "section:key". If the key cannot be found,
  the pointer passed as 'def' is returned. If value is `NULL`,
  `iniparser_null_string[] = "(null)"` will be returned instead.
  The returned char pointer is pointing to a string allocated in
  the dictionary, do not free or modify it.
 */
/*--------------------------------------------------------------------------*/
const char * iniparser_getstring(const dictionary * d, const char * key, const char * def);

This will allow explicit pointer check without changing API.