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

Fix RTSP basic auth #9870

Closed
wants to merge 2 commits into from
Closed

Fix RTSP basic auth #9870

wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Nov 8, 2022

known bug 6.8

bagder and others added 2 commits November 8, 2022 15:35
Verified with test 3100

Fixes #4750
Closes #....
@bagder bagder added the RTSP label Nov 8, 2022
@bagder bagder closed this in 2bc04d4 Nov 9, 2022
@bagder bagder deleted the bagder/rtsp-basic-auth branch November 9, 2022 08:40
@janssen70
Copy link

Once more a comment on closed PR... Seems to make sense though
Given that there has been discussion on strdup-usage before:
The unconditional free/strdup implies that on an rtsp-interleaved link (i.e. one that repeats CURL_RTSPREQ_RECEIVE) memory overhead happens a few tens to a few thousand times per second depending on payload size and presence (absence) of server-side interleaving optimizations. From that point of view it's better to assign first_host only when it is not set.

@bagder
Copy link
Member Author

bagder commented Nov 9, 2022

Agreed. It's even strange that it can update the first host...

bagder added a commit that referenced this pull request Nov 10, 2022
Suggested-by: Erik Janssen
URL: #9870 (comment)
bagder added a commit that referenced this pull request Nov 10, 2022
Suggested-by: Erik Janssen
URL: #9870 (comment)
Closes #9882
* safe assumption as no other redirects should happen from RTSP.
*/
if(conn->handler->protocol & CURLPROTO_RTSP)
data->set.redir_protocols = CURLPROTO_RTSP;
Copy link
Contributor

@monnerat monnerat Nov 10, 2022

Choose a reason for hiding this comment

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

This probably breaks reuse of handle with another protocol.
If handle is used for RTSP then for HTTP, redirection won't work on HTTP unless reset externally between both uses.
I would rather use a copy of set.redir_protocols into a new state.redir_protocols field in the whole library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yikes, that is indeed a good remark. I need to fix this...

Copy link
Member Author

Choose a reason for hiding this comment

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

The RTSP code also wrongly sets data->set.opt_no_body in several places...

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not see ithem before but you're right. I noticed this particular one while rebasing the sieve PR, as it also features redirection.

I've just done a quick check for more and there may be some (although not all listed):

$ find lib -name '*.c' | xargs grep -e '->set\.[A-Za-z0-9_.]* *= ' | grep -Fv 'setopt.c
> url.c'
lib/easy.c:    dst->set.postfields = dst->set.str[i];
lib/easy.c:  outcurl->set.buffer_size = data->set.buffer_size;
lib/http.c:      data->set.redir_protocols = CURLPROTO_RTSP;
lib/ftp.c:    data->set.ftp_filemethod = FTPFILE_MULTICWD;
lib/ftp.c:  data->set.fwrite_func = Curl_ftp_parselist;
lib/ftp.c:  data->set.out = data;
lib/ftp.c:      data->set.fwrite_func = ftpwc->backup.write_function;
lib/ftp.c:      data->set.out = ftpwc->backup.file_descriptor;
lib/doh.c:    doh->set.fmultidone = doh_done;
lib/doh.c:    doh->set.dohfor = data; /* identify for which transfer this is done */
lib/rtsp.c:  data->set.opt_no_body = TRUE; /* most requests don't contain a body */
lib/rtsp.c:    data->set.opt_no_body = FALSE;
lib/rtsp.c:    data->set.opt_no_body = FALSE;
lib/rtsp.c:    data->set.opt_no_body = FALSE;
lib/rtsp.c:      data->set.opt_no_body = TRUE;
lib/transfer.c:      data->set.trailer_data = NULL;
lib/transfer.c:      data->set.trailer_callback = NULL;
lib/transfer.c:      data->set.upload = false;
lib/http2.c:        node->data->set.stream_depends_on = child;
lib/http2.c:      parent->set.stream_dependents = 0;
lib/http2.c:      (*tail)->data->set.stream_depends_e = FALSE;
lib/http2.c:  child->set.stream_depends_on = parent;
lib/http2.c:  child->set.stream_depends_e = exclusive;
lib/http2.c:      parent->set.stream_dependents = data->next;
lib/http2.c:  child->set.stream_depends_on = 0;
lib/http2.c:  child->set.stream_depends_e = FALSE;
lib/multi.c:  data->state.conn_cache->closure_handle->set.timeout = data->set.timeout;
lib/conncache.c:  connc->closure_handle->set.buffer_size = READBUFFER_MIN;
lib/vtls/vtls.c:  data->set.general_ssl.max_ssl_sessions = amount;
$

Maybe we can plan to have only setopt values in set and r/w runtime values in state? Just an idea...

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can plan to have only setopt values in set and r/w runtime values in state?

I think we should, yes. And then possibly invent and set up some some kind of framework/markup or something that lets us scan for mistakes like the present.

Copy link
Member Author

Choose a reason for hiding this comment

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

#9888 addresses parts of this

Copy link
Contributor

@monnerat monnerat Nov 11, 2022

Choose a reason for hiding this comment

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

And then possibly invent and set up some some kind of framework/markup

Suggestion:

  • declare set as const
  • use a cast with a comment in the few functions allowed to alter it.

Not very elegant but will cause a compile error if such a mistake occurs again. The C language does not offer many alternatives to achieve that. And it does not require any external test or tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants