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

JanFellner · 2021-10-28T15:23:06Z

Follow up from this one:
#2791

…t large enough

lminiero · 2021-10-28T15:33:56Z

Why not use realloc instead of a new malloc+memcpy?

JanFellner · 2021-10-28T15:42:29Z

Can also use realloc. Will update it later. Other comments?

lminiero · 2021-10-29T07:59:04Z

Other comment would be this should probably be a core method, like the one @tmatth sketched so far, so that we can use it in other places where we currently use g_strlcat. For this reason, my idea is to merge that first (once we've reviewed it), and then we can modify the new janus_strlcat with a dynamic buffer as you said.

We should ensure this is never used on static buffers, though: in this case we're using g_malloc for the buffer, but there are areas in the code where we also use it with static string arrays, where a reallocation would mess things up.

for resizing now either using the required length or we double the size

JanFellner · 2021-10-29T08:06:52Z

Changed it to realloc and improved it in case the string is larger than the "static buffer length increase" it failed
So we either use the required string length or we double the size.
(doubling the size was the one i used in other project for dynamic printfs as using static size led to multiple resizes in the end)

lminiero · 2021-10-29T08:30:16Z

I think doubling the size is dangerous, because it means exponentially growing the buffer when something goes wrong. I'd personally just increase the buffer to what's required in that specific call.

JanFellner · 2021-10-29T09:05:08Z

If we just increase by the size we need that leads to super many reallocs. This is not time critical for the SDP answer but if the function is generally used this might have impacts.
I have good experience using the doubling the size in other projects to reduce the realloc requests (> 5.000k User projects on one server)
So i am fine with any approach just let me know what you prefer to solve the issue :)
(I see just increasing by the amount we need ressource consuming)

atoppi · 2021-10-29T09:33:22Z

I agree with @lminiero about being SUPER cautious when using this helper with stack allocated buffers.
Trying to reallocate stack memory could lead to unexpected (and very difficult to debug) crashes.
Neither the compiler nor the IDE will prevent this error that could be easily overlooked by the programmer.

I also agree with @JanFellner about getting the MAX(2*size, required_size), increasing the buffer at each call will just slowdown the path in case of large SPDs.

Finally what is missing here is a maximum value. We should not let SDPs grow to undefined sizes.

JanFellner · 2021-10-29T10:23:24Z

I´ll add a maximum, the question is then what to do in case we reach it.
Writing an error sounds logic, but should we throw an out of mem exception?
Should we just continue with the not catenated string?

atoppi · 2021-10-29T10:32:09Z

Should we just continue with the not catenated string?

I'd say no but I understand it's very redundant to check for a truncation (retval >= dest_size) anytime we use the helper.
If you think (and others also) that checking the retval for every call is acceptable, then outputting an error and aborting the current operation should be straightforward.

…ocated

JanFellner · 2021-10-29T10:43:25Z

Changed the variable name to strengthen that the buffer must be dynamically (heap) allocated.
Added a check that we do not realloc if the size is not increased.

⚠In case the buffer is exceeded, every further cat will create an error but without a reference where it actually failed. I just mentions the required and the maximum buffer size.

❓ The cat is partly executed like now or should we not cat that element at all?
❓ Should the error message contain pieces of the text (like first 50/100 bytes) to have an idea where the error comes from?

Or should we target these two questions with the janus_strcat #2792 and output related errors there? (e.g. first 50 bytes of the target buffer, first 50 bytes of the appendum, otherwise it might be hard to analyze if the method is widely used)

JanFellner · 2021-11-02T08:07:29Z

Just to have it mentioned:
Having thought about it the weekend. I would not set a maximum as it will just lead to unpredictable behavior.
I would prefer a dump showing me what went wrong instead of having a system randomly doing strange things.
The implementations i made in the past that dynamically resize buffers do not limit the buffer size.
(It might make sense to show a warning or even an error if the buffer gets too large but i would not programmatically limit it)
Any chance we get that merged today so i can merge it into the REMB branch?

lminiero · 2021-11-02T09:00:35Z

Systematic core dump on a maximum means leaving a door open to just anyone willingly crashing the system: all they need to do is have Janus craft a huge SDP and they know it will crash. I'll never add something like that, sorry.

JanFellner · 2021-11-02T09:09:00Z

I am fine with anything. Just mentioned my concerns.
Just tell me how to proceed with the PR.
The function is currently used while creating the SDP answer so the data has been already passed through janus and janus is now creating the answer on the offer.
I mentioned my open points in the message above #2793 (comment)

lminiero · 2021-11-03T11:36:30Z

I just merged #2792 and aligned multistream to use it too. Now we can check what the best way to address it is. I think we'll need a different method from janus_strlcat for that, as there's a lot of places where we use static string arrays as arguments to that method, meaning a new allocation would break things. We could add a new argument to tell the method whether the buffer is static or dynamic (e.g., a boolean), but that would mean changing a ton of lines, so a janus_strlcat_dynamic alternative for the few instances when we want to use an allocated buffer is probably a better option.

lminiero

I added some notes after merging #2792.

lminiero · 2021-11-03T15:52:23Z

sdp-utils.c

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


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.

lminiero · 2021-11-03T15:53:20Z

sdp-utils.c

+/*! \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


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.

lminiero · 2021-11-03T15:56:36Z

sdp-utils.c

+ * @param size - Current size of the buffer
+ * @returns the size of the buffer (may exceed size if it was resized)
+ */
+gsize dynamic_strlcat(char** dynamic_buffer, const char* add, gsize size) {


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.

lminiero · 2021-11-03T15:56:57Z

sdp-utils.c

+ * @returns the size of the buffer (may exceed size if it was resized)
+ */
+gsize dynamic_strlcat(char** dynamic_buffer, const char* add, gsize size) {
+	// What is the resulting length (current used buffer size + length of the addition + null byte)


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

lminiero · 2021-11-03T15:57:39Z

sdp-utils.c

+	// 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);


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

lminiero · 2021-11-03T15:58:57Z

sdp-utils.c

+		if(required > MAX_JANUS_BUFSIZE)
+			JANUS_LOG(LOG_ERR, "dynamic_strlcat - Required buffer size %ld exceeds MAX_JANUS_BUFSIZE (%d)", required, MAX_JANUS_BUFSIZE);
+		// Either we double the size, or we increase by the required size
+		size_t newsize = MAX(size * 2, required);


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 😄

lminiero · 2021-11-03T15:59:22Z

sdp-utils.c

+		// Either we double the size, or we increase by the required size
+		size_t newsize = MAX(size * 2, required);
+		if(newsize > MAX_JANUS_BUFSIZE)
+			newsize = MAX_JANUS_BUFSIZE;


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

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.

Makes sense, thanks for the clarification!

lminiero · 2021-11-03T16:02:03Z

sdp-utils.c

+			*dynamic_buffer = g_realloc(*dynamic_buffer, size);
+		}
+	}
+	g_strlcat(*dynamic_buffer, add, size);


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.

lminiero · 2021-11-03T16:02:35Z

sdp-utils.c

 			temp2 = temp2->next;
 		}
 		temp = temp->next;
 	}
 	janus_refcount_decrease(&imported->ref);
+


This extra empty line is not needed.

lminiero · 2021-11-03T16:03:43Z

PS: please let me know if you'd like me to address this transition instead (especially considering there's conflicts to address too).

JanFellner · 2021-11-04T08:01:05Z

PS: please let me know if you'd like me to address this transition instead (especially considering there's conflicts to address too).

Yeah Lorenzo, for the complexity of the function and the amount of communication we would both need just go ahead :).
I just wanted to solve the issue for me for the other feature branch. So no matter how we address it (you can also rip and replace it), go for it ;]

I go for all your proposed changes, the variable naming was my failure as i tried to rename the arguments to better clearify that the buffer needs to be dynamic but forgot about the header there.

❗ Just check the only comment about the compare against the max buffer size #2793 (comment)

JanFellner · 2021-11-04T08:03:44Z

And then burry the PR... ⚰

lminiero · 2021-11-04T15:09:20Z

I just noticed that this patch was for master and not multistream. In multistream we already use realloc for the SDP, so it's not needed there: I can use the same pattern we use there in master too, which would be simpler and wouldn't require a separate function.

lminiero · 2021-11-04T15:36:42Z

@tmatth @JanFellner can you check if the above PR fixes it for you? I tried to replicate the issue myself, but I don't have as many interfaces as Jan (assing STUN/TURN gathering helped a bit but not much), so I couldn't grow m-lines to the point they were needed.

JanFellner · 2021-11-05T10:08:01Z

checking...

JanFellner · 2021-11-05T10:51:02Z

Working. Please close it and merge the other one :)

lminiero · 2021-11-05T10:52:28Z

Working. Please close it and merge the other one :)

Ack, thanks! I'll make a couple of other tests later (just to be sure we don't find regressions) and then merge.

…2797)

lminiero · 2021-11-08T11:18:09Z

Closing, thanks for notifying about the issue and proposing a fix!

Dynamically resized the buffer for the sdp answer if the buffer is no…

49e56a8

…t large enough

tmatth mentioned this pull request Oct 28, 2021

utils: add janus_strlcat helper #2792

Merged

Now using realloc

14f4954

for resizing now either using the required length or we double the size

Added a max buffer size (1Mbyte)

4456941

JanFellner added 2 commits October 29, 2021 12:34

Changed naming to express that the memory needs to be dynamically all…

ce7fa5c

…ocated

Resize only if the buffer is really larger than before

1001fac

Machosh mentioned this pull request Nov 2, 2021

Empty (broken?) candidate in janus SDP anwer on publisher joining #2791

Closed

lminiero reviewed Nov 3, 2021

View reviewed changes

lminiero added a commit that referenced this pull request Nov 4, 2021

Grow buffer as needed when generating SDPs (fixes #2791, see #2793)

ed90055

lminiero mentioned this pull request Nov 4, 2021

Grow buffer as needed when generating SDPs (fixes #2791, see #2793) #2797

Merged

lminiero added a commit that referenced this pull request Nov 8, 2021

Grow buffer as needed when generating SDPs (fixes #2791, see #2793) (#…

326b00c

…2797)

lminiero closed this Nov 8, 2021

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

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

Conversation

JanFellner commented Oct 28, 2021

lminiero commented Oct 28, 2021

JanFellner commented Oct 28, 2021

lminiero commented Oct 29, 2021

JanFellner commented Oct 29, 2021 • edited Loading

lminiero commented Oct 29, 2021

JanFellner commented Oct 29, 2021 • edited Loading

atoppi commented Oct 29, 2021

JanFellner commented Oct 29, 2021

atoppi commented Oct 29, 2021

JanFellner commented Oct 29, 2021 • edited Loading

JanFellner commented Nov 2, 2021 • edited Loading

lminiero commented Nov 2, 2021

JanFellner commented Nov 2, 2021

lminiero commented Nov 3, 2021

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero commented Nov 3, 2021

JanFellner commented Nov 4, 2021

JanFellner commented Nov 4, 2021

lminiero commented Nov 4, 2021

lminiero commented Nov 4, 2021

JanFellner commented Nov 5, 2021

JanFellner commented Nov 5, 2021

lminiero commented Nov 5, 2021

lminiero commented Nov 8, 2021

JanFellner commented Oct 29, 2021 •

edited

Loading

JanFellner commented Oct 29, 2021 •

edited

Loading

JanFellner commented Oct 29, 2021 •

edited

Loading

JanFellner commented Nov 2, 2021 •

edited

Loading