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.

Sign up for GitHub

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

Jump to bottom

Dynamically resized the buffer for the sdp answer if the buffer is not large enough #2793

Closed
wants to merge 5 commits into from
Closed

Dynamically resized the buffer for the sdp answer if the buffer is not large enough #2793

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49e56a8
Dynamically resized the buffer for the sdp answer if the buffer is no…
JanFellner Oct 28, 2021
14f4954
Now using realloc
JanFellner Oct 29, 2021
4456941
Added a max buffer size (1Mbyte)
JanFellner Oct 29, 2021
ce7fa5c
Changed naming to express that the memory needs to be dynamically all…
JanFellner Oct 29, 2021
1001fac
Resize only if the buffer is really larger than before
JanFellner Oct 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 47 additions & 16 deletions sdp-utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
#include "debug.h"

#define JANUS_BUFSIZE 8192
#define MAX_JANUS_BUFSIZE 1048576

gsize dynamic_strlcat(char** buffer, const char* add, gsize size);
Copy link
Member

Choose a reason for hiding this comment

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

This belongs to utils.c (and the related signature in utils.h) as otherwise no other part of the code except the SDP utils will be able to use this (even though this is the only place we'll use this initially).

I'd also rename it to something like janus_strlcat_dynamic or janus_strlcat_realloc, to make it more consistent with the existing janus_strlcat and make the difference in functionality more obvious.


/* Preferred codecs when negotiating audio/video, and number of supported codecs */
const char *janus_preferred_audio_codecs[] = {
Expand Down Expand Up @@ -906,31 +909,58 @@ const char *janus_sdp_get_fmtp(janus_sdp *sdp, int pt) {
return NULL;
}

/*! \brief Concatenates a string to another and dynamically increases the buffer if it is too small
* @param dynamic_buffer - Pointer to the dynamic buffer for the target string. This must be a dynamic allocated memory.
* @param add - String to add
* @param size - Current size of the buffer
Copy link
Member

Choose a reason for hiding this comment

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

Just as a note, the - (dash) is not needed, doxygen will figure out what's the variable and what's the description.

That said, this description will only need to be added to utils.h, where the signature will be.

* @returns the size of the buffer (may exceed size if it was resized)
*/
gsize dynamic_strlcat(char** dynamic_buffer, const char* add, gsize size) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd use the same names for the variables in the signature of janus_strlcat, for consistency.

On a side note, shouldn't the last argument be a pointer, i.e., gsize *size? This would allow the method to update the current size of the dynamic buffer dynamically using an input/output variable. I prefer this to re-assigning the variable from the return value as you do now, as in case we reach the maximum size, the returned value will be higher (which is what Tristan used as a way to detect failures).

In that case, we can change the return value to something different, e.g., an integer where 0 means success and a negative value an error, or a boolean: this way, we can detect failures from the calling side, and react consequently.

// What is the resulting length (current used buffer size + length of the addition + null byte)
Copy link
Member

Choose a reason for hiding this comment

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

Code style: comments should all be in /* */ form.

size_t required = strlen(*dynamic_buffer) + strlen(add) + 1;
// In case the buffer is not large enough
if(required > size) {
if(required > MAX_JANUS_BUFSIZE)
JANUS_LOG(LOG_ERR, "dynamic_strlcat - Required buffer size %ld exceeds MAX_JANUS_BUFSIZE (%d)", required, MAX_JANUS_BUFSIZE);
Copy link
Member

Choose a reason for hiding this comment

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

There should be a return here as well: no point in going further.

// Either we double the size, or we increase by the required size
size_t newsize = MAX(size * 2, required);
Copy link
Member

Choose a reason for hiding this comment

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

Googling around, it looks like a more reasonable way to increase the buffer is to use a 1.5 grow factor, rather than two. Doubling the buffer does make me a bit nervous 😄

if(newsize > MAX_JANUS_BUFSIZE)
newsize = MAX_JANUS_BUFSIZE;
Copy link
Member

Choose a reason for hiding this comment

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

I guess this check isn't needed anymore, if we stop before that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The max is checking whether doubling (or 1.5ing) is bigger than required.
So the resulting newsize may actually be bigger than the maximum allowed buffer size in case:
where the size*1.5 is larger but the required is just less than the maximum buffer size.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks for the clarification!

if(newsize > size) {
size = newsize;
*dynamic_buffer = g_realloc(*dynamic_buffer, size);
}
}
g_strlcat(*dynamic_buffer, add, size);
Copy link
Member

Choose a reason for hiding this comment

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

I'd still check what the return value of this function is, just in case there can be other causes of issues. Maybe we can do something like this instead:

return janus_strlcat(*dynamic_buffer, add, size);

since that method has the necessary check in place, and so we can delegate it to that.

return size;
}

char *janus_sdp_write(janus_sdp *imported) {
if(!imported)
return NULL;
janus_refcount_increase(&imported->ref);
char *sdp = g_malloc(JANUS_BUFSIZE), buffer[512];
gsize bufsize = JANUS_BUFSIZE;
char *sdp = g_malloc(bufsize), buffer[512];
*sdp = '\0';
/* v= */
g_snprintf(buffer, sizeof(buffer), "v=%d\r\n", imported->version);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
/* o= */
g_snprintf(buffer, sizeof(buffer), "o=%s %"SCNu64" %"SCNu64" IN %s %s\r\n",
imported->o_name, imported->o_sessid, imported->o_version,
imported->o_ipv4 ? "IP4" : "IP6", imported->o_addr);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
/* s= */
g_snprintf(buffer, sizeof(buffer), "s=%s\r\n", imported->s_name);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
/* t= */
g_snprintf(buffer, sizeof(buffer), "t=%"SCNu64" %"SCNu64"\r\n", imported->t_start, imported->t_stop);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
/* c= */
if(imported->c_addr != NULL) {
g_snprintf(buffer, sizeof(buffer), "c=IN %s %s\r\n",
imported->c_ipv4 ? "IP4" : "IP6", imported->c_addr);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
}
/* a= */
GList *temp = imported->attributes;
Expand All @@ -941,61 +971,61 @@ char *janus_sdp_write(janus_sdp *imported) {
} else {
g_snprintf(buffer, sizeof(buffer), "a=%s\r\n", a->name);
}
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
temp = temp->next;
}
/* m= */
temp = imported->m_lines;
while(temp) {
janus_sdp_mline *m = (janus_sdp_mline *)temp->data;
g_snprintf(buffer, sizeof(buffer), "m=%s %d %s", m->type_str, m->port, m->proto);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
if(m->port == 0 && m->type != JANUS_SDP_APPLICATION) {
/* Remove all payload types/formats if we're rejecting the media */
g_list_free_full(m->fmts, (GDestroyNotify)g_free);
m->fmts = NULL;
g_list_free(m->ptypes);
m->ptypes = NULL;
m->ptypes = g_list_append(m->ptypes, GINT_TO_POINTER(0));
g_strlcat(sdp, " 0", JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, " 0", bufsize);
} else {
if(m->proto != NULL && strstr(m->proto, "RTP") != NULL) {
/* RTP profile, use payload types */
GList *ptypes = m->ptypes;
while(ptypes) {
g_snprintf(buffer, sizeof(buffer), " %d", GPOINTER_TO_INT(ptypes->data));
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
ptypes = ptypes->next;
}
} else {
/* Something else, use formats */
GList *fmts = m->fmts;
while(fmts) {
g_snprintf(buffer, sizeof(buffer), " %s", (char *)(fmts->data));
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
fmts = fmts->next;
}
}
}
g_strlcat(sdp, "\r\n", JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, "\r\n", bufsize);
/* c= */
if(m->c_addr != NULL) {
g_snprintf(buffer, sizeof(buffer), "c=IN %s %s\r\n",
m->c_ipv4 ? "IP4" : "IP6", m->c_addr);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
}
if(m->port > 0) {
/* b= */
if(m->b_name != NULL) {
g_snprintf(buffer, sizeof(buffer), "b=%s:%"SCNu32"\r\n", m->b_name, m->b_value);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
}
}
/* a= (note that we don't format the direction if it's JANUS_SDP_DEFAULT) */
const char *direction = m->direction != JANUS_SDP_DEFAULT ? janus_sdp_mdirection_str(m->direction) : NULL;
if(direction != NULL) {
g_snprintf(buffer, sizeof(buffer), "a=%s\r\n", direction);
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
}
GList *temp2 = m->attributes;
while(temp2) {
Expand All @@ -1010,12 +1040,13 @@ char *janus_sdp_write(janus_sdp *imported) {
} else {
g_snprintf(buffer, sizeof(buffer), "a=%s\r\n", a->name);
}
g_strlcat(sdp, buffer, JANUS_BUFSIZE);
bufsize = dynamic_strlcat(&sdp, buffer, bufsize);
temp2 = temp2->next;
}
temp = temp->next;
}
janus_refcount_decrease(&imported->ref);

Copy link
Member

Choose a reason for hiding this comment

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

This extra empty line is not needed.

return sdp;
}

Expand Down