Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sendf: accept zero-length data in Curl_client_write() #7898

Closed
wants to merge 1 commit into from

Conversation

monnerat
Copy link
Contributor

As Curl_client_write() rejects zero-length output data, ldap root
requests resulting in a null distinguished name currently cause curl to
abort.

This commit explicitly checks for non zero-length DN before writing it
to LDIF output.

This is the first part of splitting PR #7199 (that still allows me to run CI).

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean the assert(len) that was added in #6954. IMO it's easier to just return CURLE_OK on !len rather than check every time before we call it that it's not 0, but I'm fine either way.

@jay jay added the LDAP label Oct 23, 2021
@bagder
Copy link
Member

bagder commented Oct 23, 2021

I too would be fine with converting the assert to a plain check and return. I think the assert may have served it purpose by now.

@monnerat
Copy link
Contributor Author

monnerat commented Oct 23, 2021

If you mean the assert(len) that was added in #6954 ...

Yes, this is the cause of the abort.

I too would be fine with converting the assert to a plain check and return.

Would you like I convert this PR to do it? In this case I would also be in favor of changing the next line:
DEBUGASSERT(type <= 3);
into
DEBUGASSERT(!(type & ~CLIENTWRITE_BOTH));
which performs an equivalent check but with clearer info.

@jay
Copy link
Member

jay commented Oct 24, 2021

ok

@bagder
Copy link
Member

bagder commented Oct 25, 2021

I'm fine with that as well!

Historically, Curl_client_write() used a length value of 0 as a marker
for a null-terminated data string. This feature has been removed in
commit f4b85d2. To detect leftover uses of the feature, a DEBUGASSERT
statement rejecting a length with value 0 was introduced, effectively
precluding use of this function with zero-length data.

The current commit removes the DEBUGASSERT and makes the function to
return immediately if length is 0.

A direct effect is to fix trying to output a zero-length distinguished
name in openldap.

Another DEBUGASSERT statement is also rephrased for better readability.
@monnerat monnerat changed the title openldap: fix a zero length client output sendf: accept zero-length data in Curl_client_write() Oct 25, 2021
@monnerat
Copy link
Contributor Author

Commit replaced. PR conversion done.

@monnerat monnerat requested a review from jay October 25, 2021 12:19
@bagder bagder closed this in fa84ce3 Oct 25, 2021
@bagder
Copy link
Member

bagder commented Oct 25, 2021

Thanks!

@monnerat
Copy link
Contributor Author

Thanks for pulling.

@monnerat monnerat deleted the openldap-zero-length branch October 25, 2021 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants