Segfault in rtsp.c when using WRITEDATA #1880

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

Comments

Projects
None yet
2 participants
@cmeister2
Contributor

cmeister2 commented Sep 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@cmeister2

cmeister2 Sep 11, 2017

Contributor

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);
Contributor

cmeister2 commented Sep 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 12, 2017

Member

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?

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?

cmeister2 added a commit to cmeister2/curl that referenced this issue Sep 12, 2017

rtsp: Segfault in rtsp.c when using WRITEDATA
If the INTERLEAVEFUNCTION is defined, then use that plus the
INTERLEAVEDATA information when writing RTP. Otherwise, use
WRITEFUNCTION and WRITEDATA.

Fixes #1880

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