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

DoH requests do not send verbose output to the original request's debug function #6605

Closed
arvids-kokins-bidstack opened this issue Feb 14, 2021 · 7 comments
Labels

Comments

@arvids-kokins-bidstack
Copy link

As noted in the title, it was expected that any verbose output from the request (including DoH) would go to the same debug function instead of being dumped into the default destination (stderr) so that all of the output can be properly redirected.

example patch commit: bidstack-group@125c1a8
this is probably not the complete fix though, as I see there's also CURLOPT_STDERR which isn't copied over to the DoH request as well (and possibly other things)

@jay jay added the name lookup DNS and related tech label Feb 14, 2021
@jay
Copy link
Member

jay commented Feb 14, 2021

At some point we discussed support for DOH handles inheriting from CURLOPT_STDERR and CURLOPT_DEBUGFUNCTION but the problem is the user may close the handle they were inherited from and the data no longer valid. For example, the user opens a file to write stderr, their transfer is done so they clean up the curl handle and close the file handle, meanwhile the DOH handles still have that file handle. I'm almost certain we discussed it in #3661 but I guess some commit comments may have disappeared when I rebased.

@arvids-kokins-bidstack
Copy link
Author

the problem is the user may close the handle they were inherited from

Doesn't the DoH request get closed in that case as well? I'm still trying to understand how everything works there, but it seemed like DoH handles are closed either when the lookup is done (https://github.com/curl/curl/blob/master/lib/doh.c#L935) - which should happen before the transfer is done - or when the owning request is closed (https://github.com/curl/curl/blob/master/lib/url.c#L396).

@jay
Copy link
Member

jay commented Feb 14, 2021

Yes that's possible. I can't find the conversation so I'm not sure exactly why we didn't add it.

@bagder
Copy link
Member

bagder commented Feb 14, 2021

I believe something that originally made me hesitate was this: we set verbose mode and the debug callback on a per-handle basis. The user then probably expects that the messages that are sent to the callback are related to that particular easy handle. When we use DoH, we create sub-easy handles within libcurl itself for the underlying DoH requests and the verbose messages for those transfers would then be "anchored" on those easy handles and not the original one for which the user set the options. Then the debug callback would get called for other easy handles and while I don't think we ever has said that can't happen and we haven't forbidden it, it is still something I think maybe we need to highlight and caution for in the documentation if we make this inherited by default.

We also have the option of adding another bit to the verbose option or something to ask users to explicitly opt-in to this behavior.

jay added a commit to jay/curl that referenced this issue Feb 25, 2021
Prior to this change if the user set their easy handle's error stream
to something other than stderr it was not inherited by the doh handles,
which meant that they would still write to the default standard error
stream (stderr) for verbose output.

Bug: curl#6605
Reported-by: arvids-kokins-bidstack@users.noreply.github.com

Closes #xxxx
jay added a commit to jay/curl that referenced this issue Feb 25, 2021
Prior to this change if the user set their easy handle's error stream
to something other than stderr it was not inherited by the doh handles,
which meant that they would still write to the default standard error
stream (stderr) for verbose output.

Bug: curl#6605
Reported-by: arvids-kokins-bidstack@users.noreply.github.com

Closes #xxxx
@jay
Copy link
Member

jay commented Feb 25, 2021

I'm proposing #6661 to deal with this partially. The stderr setting will be inherited from the user's easy handle but not the debug function. A way to solve this fully I think would be to have a callback for advanced users to allow them to set settings on the DOH handles, rather than have extra _DOH prefixed options for everything, so that way they could override some settings.

@arvids-kokins-bidstack
Copy link
Author

The proposed callback would probably solve our issue. 👍

The stderr setting will be inherited from the user's easy handle but not the debug function.

What's the differentiating aspect between stderr and the debug function that has led to such a decision?

@jay
Copy link
Member

jay commented Mar 2, 2021

The stderr setting will be inherited from the user's easy handle but not the debug function.

What's the differentiating aspect between stderr and the debug function that has led to such a decision?

As @bagder mentioned earlier some users may have written debug functions with logic that only expects messages from their single handle and therefore they do not check the handle. stderr redirection is different because though additional verbose output may be unexpected it is not potentially catastrophic.

jay added a commit that referenced this issue Mar 2, 2021
Prior to this change if the user set their easy handle's error stream
to something other than stderr it was not inherited by the doh handles,
which meant that they would still write to the default standard error
stream (stderr) for verbose output.

Bug: #6605
Reported-by: arvids-kokins-bidstack@users.noreply.github.com

Closes #6661
@jay jay closed this as completed in 65aa275 Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants