[Asterisk-code-review] term: truncate the message rather than the escapes. (asterisk[master])

[Jaco Kroon]

Attention is currently required from: Sean Bright, Joshua Colp.
Jaco Kroon has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16640 )

Change subject: term: truncate the message rather than the escapes.
......................................................................


Patch Set 3:

(1 comment)

Patchset:

PS3: 
> Can you please tell me under what circumstances this has actually been a problem? What invocation of […]
Hi Sean,

As indicated on IRC, external (out of tree) C++ modules.  As discussed there are a few more potential improvements that can be made to this patch, but as it stands, the "worse code path" will only affect the case where the buffer gets overrun.

We've had that in one of our own modules with a fairly mundane function name, but with a somewhat large number of arguments.  Simply making the buffer larger merely delays the problem, making it dynamic worsens the code path in all cases (and just results in nearly unreadable logs anyway, so limiting the buffer does make sense).

The primary concept I think I may be able to utilize from your patch is to just use sprintf once, even for the "bad" path, whereas this patch calls it twice.  I suspect the best possible option will be to kind-of go back to my original patch, use snprintf to push in the initial escape sequence (at this point we can already determine if more than the escape sequences will fit), a second snprintf for the input buffer (so we can get the string length in the same step which strcpy won't give is requiring an extra strlen), and then truncate as required before putting in the trail.  But since we can then determine before the second snprintf the original patch (set 2) is pretty much equivalent in that case.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16640
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I80ef7a4bfd2947e090ef830143391d11baebdb0d
Gerrit-Change-Number: 16640
Gerrit-PatchSet: 3
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Sat, 13 Nov 2021 19:46:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean at seanbright.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211113/10d0cced/attachment.html>