This commit needs an explanation why it is necessary

@jay Added a commit message.

Renaming malloc as curlx_malloc requires more mental processing when skimming the code. You see malloc and you know instantly what it is. malloc and, in the rare case, (malloc) are easier to understand

This GSS API uses system free() to free these buffers

Is this documented/suggested behavior? It seems like a brittle operation to malloc in one library and have another free the memory. It would feel more appropriate if the GSS library itself had some way to do that malloc?

One issue is that it isn't actually malloc. As this bug shows. One other is that it requires fragile PP tricks, and order-dependent includes.

One issue is that it isn't actually malloc.

That's my point. libcurl handles its memory the way it wants, and another library should not free its memory. And vice versa.

Maybe we should add a gss_malloc() macro ourselves if GSS doesn't have one, that makes it clear what the malloc is for.

No, it doesn't seem like the documented way after a quick glance at https://manpages.debian.org/stretch/gss-man/gss_release_buffer.3.en.html. Haven't dug into why it's done that way. I'm thinking to close this without merge.

edit: but, there are 2 (and 3 not long ago) GSS implementations, and this was the man page of just one of them.

Yeah it says for that function that "The storage must have been allocated by a GSS-API routine." But I don't see a similar gss_malloc etc in their API. There's one piece of code that already does the (malloc) trick:

That's in the local debug stub/wrapper, so not exactly the same situation, but I met this tricky API there first. There is also the "import" variant. (I'm on a phone, not able to check anything ATM)

BTW, this mixup is also reported by Joshua:

F-110: Application-allocated gss_recv_token.value freed via gss_release_buffer (heap corruption/DoS)

• Evidence: lib/socks_gssapi.c. gss_recv_token.value is allocated with malloc but freed with gss_release_buffer.
• Rationale: This is the same lifetime violation as the previous finding but for inbound tokens, and it can corrupt the heap or crash.

Got it from there, yes. It's also F-109.

As for F-111, it looked okay. But might use a second look.

In krb5_gssapi.c, .value is also allocated with the curl allocator,
but it's also freed via curl's free, instead of calling gss_release_buffer().

Both pass the buffers as inputs to gss_import_name() (which then
creates a GSS "name", without reusing (as far as I clould trace the code)
the passed input for the returned output. Its output then needs to be released
with gss_release_name().

gss_release_buffer() internally either uses free() or the native Win32
API on Windows (only with MIT Kerberos), so I need to give this another go
to apply the pattern used in krb5_gssapi.c and avoid using gss_release_buffer()
for cases where the buffer was allocated by curl. (This API seems to be designed
to free buffers allocated and returned by a GSS API.)

I reworked this to replace the buffer release wth curl free to match
the curl allocators. (instead of using GSS's release buffer API.)

This makes this code similar to other GSS code in the repo, and also
works with MIT Kerberos Windows, which doesn't use the system
malloc/free anyway.

Reviews are welcome!

I reworked this to replace the buffer release wth curl free

Looks ok to me as long as those gss buffer objects are not used after free

I reworked this to replace the buffer release wth curl free

Looks ok to me as long as those gss buffer objects are not used after free

Indeed, though this patch doesn't change the placement of free
calls, nor does it add or remove any, it will now actually free those buffers.
Which may bring pre-existing issues to the surface, if there was one.

I replaced free() with Curl_safefree() to set the freed pointer to NULL.
To avoid a dangling pointer, and to bring it closer to the replaced API which
also did this.