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

Segfault in rtsp.c when using WRITEDATA #1880

Closed
cmeister2 opened this issue Sep 11, 2017 · 2 comments
Closed

Segfault in rtsp.c when using WRITEDATA #1880

cmeister2 opened this issue Sep 11, 2017 · 2 comments

Comments

@cmeister2
Copy link
Contributor

After the fix to rtsp.c in a14f715, I've tried to run a test where I set WRITEDATA and WRITEFUNCTION and rtsp.c is still failing. I think the issue is as below:

  writeit = data->set.fwrite_rtp?data->set.fwrite_rtp:data->set.fwrite_func;

  if(!data->set.fwrite_rtp && !data->set.is_fwrite_set &&
     !data->set.rtp_out) {
    /* if no callback is set for either RTP or default, the default function
       fwrite() is utilized and that can't handle a NULL input */
    failf(data, "No destination to default data callback!");
    return CURLE_WRITE_ERROR;
  }

  wrote = writeit(ptr, 1, len, data->set.rtp_out);

It doesn't suffice to simply use data->set.rtp_out - I think the userdata pointer has to be set up by the if statement as well. I think if the fwrite_func is being used then the data->set.out pointer should be passed to the callback instead of data->set.rtp_out.

@cmeister2
Copy link
Contributor Author

The following code appears to fix it but I'm not 100% sure if that's correct. In particular I'm not really sure where the RTP output code is set up.

  curl_write_callback writeit;
  void *user_ptr;

  if(len == 0) {
    failf(data, "Cannot write a 0 size RTP packet.");
    return CURLE_WRITE_ERROR;
  }

  if(data->set.fwrite_rtp) {
    writeit = data->set.fwrite_rtp;
    user_ptr = data->set.rtp_out;
  }
  else
  {
    writeit = data->set.fwrite_func;
    user_ptr = data->set.out;
  }

  if(!data->set.fwrite_rtp && !data->set.is_fwrite_set &&
     user_ptr == NULL) {
    /* if no callback is set for either RTP or default, the default function
       fwrite() is utilized and that can't handle a NULL input */
    failf(data, "No destination to default data callback!");
    return CURLE_WRITE_ERROR;
  }

  wrote = writeit(ptr, 1, len, user_ptr);

@bagder
Copy link
Member

bagder commented Sep 12, 2017

I did my change like that since I was convinced that CURLOPT_INTERLEAVEDATA was documented to get passed to the WRITEFUNCTION if no INTERLEAVEFUNCTION was set, but now when I read the docs again with an extra eye on this I can see no hint or indication that it would actually work like this.

But still the code does make this decision, so the question is then who's right. The code or the (lack of) docs saying it should work like that.

I think I'm inclined to rule against the code here and in favor of the documentation. So yes, let's pass the data to the regular callback + writedata unless the INTERLEAVEFUNCTION is set and then use both. The code can be simplified a bit then since it won't send a NULL to fwrite() using the default setup, so the entire extra precaution I added can be removed again:

  curl_write_callback writeit;
  void *user_ptr;

  if(len == 0) {
    failf(data, "Cannot write a 0 size RTP packet.");
    return CURLE_WRITE_ERROR;
  }

  if(data->set.fwrite_rtp) {
    writeit = data->set.fwrite_rtp;
    user_ptr = data->set.rtp_out;
  }
  else
  {
    writeit = data->set.fwrite_func;
    user_ptr = data->set.out;
  }

  wrote = writeit(ptr, 1, len, user_ptr);

Can you perhaps amend this by also making some clarification in the documentation?

@bagder bagder closed this as completed in 08dbed3 Sep 15, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

2 participants