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

@monnerat monnerat commented Oct 23, 2021

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).

jay
jay approved these changes Oct 23, 2021
Copy link
Member

@jay jay left a comment

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.

Loading

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

@bagder 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.

Loading

@monnerat
Copy link
Contributor Author

@monnerat 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.

Loading

@jay
Copy link
Member

@jay jay commented Oct 24, 2021

ok

Loading

@bagder
Copy link
Member

@bagder bagder commented Oct 25, 2021

I'm fine with that as well!

Loading

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 force-pushed the openldap-zero-length branch from 411a2b4 to b75351c Oct 25, 2021
@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

@monnerat monnerat commented Oct 25, 2021

Commit replaced. PR conversion done.

Loading

@monnerat monnerat requested a review from jay Oct 25, 2021
bagder
bagder approved these changes Oct 25, 2021
jay
jay approved these changes Oct 25, 2021
@bagder bagder closed this in fa84ce3 Oct 25, 2021
@bagder
Copy link
Member

@bagder bagder commented Oct 25, 2021

Thanks!

Loading

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Oct 25, 2021

Thanks for pulling.

Loading

@monnerat monnerat deleted the openldap-zero-length branch Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants