Congratulations 🎉. DeepCode analyzed your code in 2.831 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Suggestions:

1. Put the Curl_auth_buffer* functions into its own new file
2. Maybe call them Curl_authbuffer* - without underscore in the middle - to be consistent with the struct name? Maybe even call it something else since strictly speaking it doesn't have to be about auth at all?
3. Use accessors to get the pointer and length of data to match with how dynbuf works: Curl_authbuffer_ptr() and Curl_authbuffer_len() - that also don't expose the struct fields.
4. s/Curl_auth_buffer_alloc/Curl_authbuffer_memdup/ - to better signal that it dupes the memory
5. A unit test for the Curl_authbuffer* functions
6. Consider a debug-build-only field in the struct like the dynbuf struct has, that we can do a DEBUGASSERT on to quickly detect wrong-doings like trying to use a non-initialized struct.

To ponder:

In the dynbuf systems we enforce a maximum buffer size at all times to avoid the risk that ridiculously sized values can get set, that then will lead to badness in places (like integer overflows). An extra internal layer of defense. Would it be sensible to make the *set() function refuse to set strings larger than CURL_MAX_INPUT_LENGTH for this sake?

I restricted this to auth to avoid name space pollution, but obviously you want to make it available elsewhere by turning it into a "global" support feature and I think you're right :-) Now, how do we call it? Hard to find a representative name. curlbuffer is not meaningful, curlstorage? curlbinbuf? curlfixbuf? curlarea? curldeallocator? maybe you think of something better

Would it be sensible to make the *set() function refuse to set strings larger than CURL_MAX_INPUT_LENGTH for this sake?

Much less than for dynbuf: Unless using Curl_auth_buffer_alloc(), there's no allocation. This kind of check mainly preserves setting a crazy length value, which is used only by callers and Curl_auth_buffer_alloc(). But we can do it anyway (the drawback is we have to test a return status after each call), or at least only as a DEBUGASSERT().

obviously you want to make it available elsewhere by turning it into a "global" support feature and I think you're right :-)

Yeah, at least I want to have the ability to do so, and if/when that time comes it will be odd to have this name!

I'm not sure the exact name is terribly important as we'll learn it and associate it with the use we assign to it. It primarily keeps a reference to a (generic) buffer and size. Maybe just call it struct bufref and Curl_bufref_*() ? (As you might know, I'm also personally rather fond of keeping the names short)

Now split in 2 commits.
@bagder : All your remarks taken into account. Should be ready now.

@bagder : thanks for your review. Fixups applied according to your remarks.

@bagder : do you intend to apply this? are you waiting for the next feature window?
The problem is: I have some related work ready but not applicable before this PR:

• binary sasl
• a fix for the ntlm logic
• completely reworked openldap (state machine, starttls, sasl and immediate output + a hack/fix for Segfault in openldap while using ldaps #6676)

Do I wait for this PR to be pulled or do I include the above in this PR ?

Why do you need bufref, can't you use dynbuf? I know you said earlier realloc isn't needed but dynbuf would work well anyway wouldn't it? If separate handling is in fact needed it should have documentation. See docs/DYNBUF.md for example.

Do I wait for this PR to be pulled or do I include the above in this PR ?

Those extra things you listed sound great but not related to this. You could always branch off this PR. I'm for next feature window for this one.

Why do you need bufref, can't you use dynbuf? I know you said earlier realloc isn't needed but dynbuf would work well anyway wouldn't it?

No, you can't. bufref's goal is to hold pointers to (preallocated) data and their associated destructor, if some. This allows using static data or data allocated with possibly something else than malloc() to be handled without being copied. In addition, the destructor is set at the same time as the pointer, allowing an easy release with Curl_bufref_free() without checking the origin of the pointer and whatever the associated destructor is. The length parameter is not used by the methods: it's only here to support binary data.

If separate handling is in fact needed it should have documentation.

As it was originally a tiny sasl-private API, I did not document it. If it should be used more widely, so yes, I could write such a document.

docs/BUFREF.md added

Thanks!

Thanks for pulling.