Use gnutls_strdup() in library code

Add a description of the new feature/bug fix. Reference any relevant bugs.

Checklist

• Code modified for feature
• Test suite updated with functionality tests
• Test suite updated with negative tests
• Documentation updated

Reviewer's checklist:

• Any issues marked for closing are addressed
• There is a test suite reasonably covering new functionality or modifications
• Function naming, parameters, return values, types, etc., are consistent and according to CONTRIBUTION.md
• This feature/change has adequate documentation added
• No obvious mistakes in the code

marked the checklist item Function naming, parameters, return values, types, etc., are consistent and according to CONTRIBUTION.md as completed

marked the checklist item No obvious mistakes in the code as completed

This looks obviously correct, so approving :-)

approved this merge request

merged

mentioned in commit 90a71b2e

mentioned in issue #491 (closed)

I think that change is incorrect. It replaces strdup() with gnutls_strdup() on a function which is documented its output to be freed using free() instead of gnutls_free(). This function returns either the output of strdup or getline. As such we cannot replace strdup without replacing getline as well.

Good catch, I simply missed that, but then...

That function is an internal one, called from one place in gnutls_priority_init(). I see it also in libgnutls.map and a test C file. So we can change the docs/comment about free() at will and change the test to use gnutls_free(). WDYT ?

Using strdup() and a gnutls_strdup() in the library code just pulls in a superfluous dependency (and duplicated code from gnulib in case the libc doesn't provide strdup()). Not talking about caching and extra work when linking (if not inlined).

The problem is that this function may return the output of getline or strdup, that's why it is documented to be used with free instead of gnutls_free.

Well, _gnutls_resolve_priorities() is only used in tests/system-prio-file.c and not meant as a public function. That function can also be directly reached/tested through gnutls_priority_init(). Am I wrong ? If that is possible, we don't have to expose _gnutls_resolve_priorities() at all.

Also, instead of strdup() we can use gnutls_strdup(). Regarding getline(): the use of this function may cause issues since you can't limit the length of the input. It may be reasonable to limit the input (whatever makes sense here, 1k, 4k, 32k ?). Doing so we can use gnutls_free().

And then... the issue with free() vs. gnutls_free() occurs on Windows only (AFAIK). Since Windows doesn't provide getline(), it will be taken from gnulib and thus the code is inside the gnutls DLL. And that allows calling gnutls_free().

Well, _gnutls_resolve_priorities() is only used in tests/system-prio-file.c and not meant as a public function. That function can also be directly reached/tested through gnutls_priority_init(). Am I wrong ? If that is possible, we don't have to expose _gnutls_resolve_priorities() at all.

The reason of it being exported (as internal symbol), is for unit testing. It is not intended to be available to applications (not exported in a header).

Also, instead of strdup() we can use gnutls_strdup(). Regarding getline(): the use of this function may cause issues since you can't limit the length of the input. It may be reasonable to limit the input (whatever makes sense here, 1k, 4k, 32k ?). Doing so we can use gnutls_free().

I'm not sure if there is really a concern here (loading the system-wide config), but that could indeed be a way to use gnutls_free here.

And then... the issue with free() vs. gnutls_free() occurs on Windows only (AFAIK). Since Windows doesn't provide getline(), it will be taken from gnulib and thus the code is inside the gnutls DLL. And that allows calling gnutls_free().

That would not be the case if the application has replaced the allocation functions. The original/historical reason for gnutls_free and gnutls_malloc was so that applications could (in theory) override them. Today is mostly windows and leaving the possibility open to moving to another allocator in the future (e.g., talloc).

Should we revert this commit for 3.6.4 until we have a better solution?

Just about to push a solution...

mentioned in merge request !756 (merged)

It's MR !756 (merged). I leave getline() as is and just replaced the malloc() for the returned string by gnutls_malloc(). So that the calling function can always use gnutls_free() to release it.