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

app_rpt.c: Fix truncated link list in RPT_LINK #470

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mkmer
Copy link
Collaborator

@mkmer mkmer commented Jan 27, 2025

Fixes #469 - processing received link list text message was limited to 512 characters. Expanded to MAXLINKLIST.

@mkmer mkmer force-pushed the finddelim-fix branch 2 times, most recently from ce000cc to 7d2be48 Compare January 27, 2025 23:08
@Allan-N Allan-N changed the title app_rpt.c: Fix issue 469 - truncated link list in RPT_LINK app_rpt.c: Fix truncated link list in RPT_LINK Jan 27, 2025
@mkmer mkmer force-pushed the finddelim-fix branch 4 times, most recently from 1da8f5e to 6d8f530 Compare January 28, 2025 00:45
apps/app_rpt.c Outdated Show resolved Hide resolved
apps/app_rpt.c Outdated Show resolved Hide resolved
apps/app_rpt/rpt_link.c Outdated Show resolved Hide resolved
time(&mylink->linklistreceived);
rpt_mutex_unlock(&myrpt->lock);
ast_debug(7, "@@@@ node %s recieved node list %s from node %s\n", myrpt->name, tmp, mylink->name);
return;
}
if (tmp[0] == 'M') {
rest = 0;
if (sscanf(tmp, "%s %s %s %n", cmd, src, dest, &rest) < 3) {
if (sscanf(tmp, "%s %s %s %n", cmd, src, dest, &rest) < 3) { /*TODO: We should limit to sizeof(cmd, src, dest)*/
Copy link
Member

Choose a reason for hiding this comment

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

It seems like tmp is mutable since we already have a copy of it (I haven't fully verified though)
Instead of the existing code, maybe we should replace with strsep for the first 3 strings and just use sscanf for the numbers? That way we can fix the issue more robustly as opposed to making a TODO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't disagree we should fix this, but in the interest of keeping this PR pointed at the actual issue maybe do it in a new PR? I'm thinking there would be a "general" fix up sscanf buffer over potentials everywhere, and hopefully the same way.
As for scanf, would %299s protect us with a lighter touch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe something like this....

#define SIZE = 300
char format[20];
char one[SIZE], two[SIZE], three[SIZE]
sprintf(format,"%%%ds %%%ds %%%ds",SIZE-1,SIZE-1,SIZE-1);
sscanf(s, format, one, two, three);

apps/app_rpt.c Outdated Show resolved Hide resolved
@mkmer
Copy link
Collaborator Author

mkmer commented Jan 28, 2025

I general, this "fixes" the issue as seen. The questions that remain for a future PR (in my mind anyway):

  • What is the max string we can receive from a text message - the initial call is a malloc, copy to pass str in.
  • What is the max string a channel variable can accept? (Is it 5120 and that's why this value was chosen?) - appears that it can be any size? https://github.com/asterisk/asterisk/blob/32e6a309525cfd28133124cbe9e8c97fe9c90c52/utils/extconf.c#L2454-L2468
  • Do we need to copy it in the routine, as it's malloc, copied, and freed around the call to either routine

    app_rpt/apps/app_rpt.c

    Lines 4407 to 4413 in e957e50

    } else if (f->frametype == AST_FRAME_TEXT) {
    char *tstr = ast_malloc(f->datalen + 1);
    if (tstr) {
    memcpy(tstr, f->data.ptr, f->datalen);
    tstr[f->datalen] = 0;
    handle_link_data(myrpt, l, tstr);
    ast_free(tstr);
  • Should l->linklist be a fixed value or be sized to the data (this may not be nice as we would malloc/free for every message received to resize it...)
  • What is the "right" size for buf, or should it be malloc based on the size of the links when we build it (also depending on max channel variable size)? It could be VERY undersized if every l->linklist is MAXLINKLIST or even half.
    char buf[MAXLINKLIST], obuf[MAXLINKLIST + 20], *strs[MAXLINKLIST];

                           Add some comments around the size issues
                           Add TODO to sscanf buffer protection in the future
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPT_LINKS incomplete node list
3 participants