Misc minor fixes in reqresp_parser.c and chan_sip.c.
authorRichard Mudgett <rmudgett@digium.com>
Thu, 19 Jan 2012 23:31:17 +0000 (23:31 +0000)
committerRichard Mudgett <rmudgett@digium.com>
Thu, 19 Jan 2012 23:31:17 +0000 (23:31 +0000)
* Fix corner cases in get_calleridname() parsing and ensure that the
output buffer is nul terminated.

* Make get_calleridname() truncate the name it parses if the given buffer
is too small rather than abandoning the parse and not returning anything
for the name.  Adjusted get_calleridname_test() unit test to handle the
truncation change.

* Fix get_in_brackets_test() unit test to check the results of
get_in_brackets() correctly.

* Fix parse_name_andor_addr() to not return the address of a local buffer.
This function is currently not used.

* Fix potential NULL pointer dereference in sip_sendtext().

* No need to memset(calleridname) in check_user_full() or tmp_name in
get_name_and_number() because get_calleridname() ensures that it is nul
terminated.

* Reply with an accurate response if get_msg_text() fails in
receive_message().  This is academic in v1.8 because get_msg_text() can
never fail.
........

Merged revisions 351618 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........

Merged revisions 351646 from http://svn.asterisk.org/svn/asterisk/branches/10

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@351667 65c4cc65-6c06-0410-ace0-fbb531ad65f3

channels/chan_sip.c
channels/sip/reqresp_parser.c

index 43a9d8b..2bdd64d 100644 (file)
@@ -4480,20 +4480,26 @@ static const char *sip_get_callid(struct ast_channel *chan)
 static int sip_sendtext(struct ast_channel *ast, const char *text)
 {
        struct sip_pvt *dialog = ast->tech_pvt;
-       int debug = sip_debug_test_pvt(dialog);
+       int debug;
 
-       if (!dialog)
+       if (!dialog) {
                return -1;
+       }
        /* NOT ast_strlen_zero, because a zero-length message is specifically
         * allowed by RFC 3428 (See section 10, Examples) */
-       if (!text)
+       if (!text) {
                return 0;
+       }
        if(!is_method_allowed(&dialog->allowed_methods, SIP_MESSAGE)) {
                ast_debug(2, "Trying to send MESSAGE to device that does not support it.\n");
                return(0);
        }
-       if (debug)
+
+       debug = sip_debug_test_pvt(dialog);
+       if (debug) {
                ast_verbose("Sending text %s on %s\n", text, ast_channel_name(ast));
+       }
+
        transmit_message_with_text(dialog, text, 0, 0);
        return 0;
 }
@@ -16318,7 +16324,6 @@ static enum check_auth_result check_user_full(struct sip_pvt *p, struct sip_requ
 
        ast_copy_string(from, sip_get_header(req, "From"), sizeof(from));
        /* XXX here tries to map the username for invite things */
-       memset(calleridname, 0, sizeof(calleridname));
 
        /* strip the display-name portion off the beginning of the FROM header. */
        if (!(of = (char *) get_calleridname(from, calleridname, sizeof(calleridname)))) {
@@ -16538,9 +16543,10 @@ static void receive_message(struct sip_pvt *p, struct sip_request *req, struct a
 
        if (get_msg_text2(&buf, req)) {
                ast_log(LOG_WARNING, "Unable to retrieve text from %s\n", p->callid);
-               transmit_response(p, "202 Accepted", req);
-               if (!p->owner)
+               transmit_response(p, "500 Internal Server Error", req);
+               if (!p->owner) {
                        sip_scheddestroy(p, DEFAULT_TRANS_TIMEOUT);
+               }
                return;
        }
 
index d135e00..b646f7b 100644 (file)
@@ -685,63 +685,77 @@ const char *get_calleridname(const char *input, char *output, size_t outputsize)
        char *orig_output = output;
        const char *orig_input = input;
 
+       if (!output || !outputsize) {
+               /* Bad output parameters.  Should never happen. */
+               return input;
+       }
+
        /* clear any empty characters in the beginning */
        input = ast_skip_blanks(input);
 
-       /* no data at all or no storage room? */
-       if (!input || *input == '<' || !outputsize || !output) {
-               return orig_input;
-       }
-
        /* make sure the output buffer is initilized */
        *orig_output = '\0';
 
        /* make room for '\0' at the end of the output buffer */
-       outputsize--;
+       --outputsize;
+
+       /* no data at all or no display name? */
+       if (!input || *input == '<') {
+               return input;
+       }
 
        /* quoted-string rules */
        if (input[0] == '"') {
                input++; /* skip the first " */
 
-               for (;((outputsize > 0) && *input); input++) {
+               for (; *input; ++input) {
                        if (*input == '"') {  /* end of quoted-string */
                                break;
                        } else if (*input == 0x5c) { /* quoted-pair = "\" (%x00-09 / %x0B-0C / %x0E-7F) */
-                               input++;
-                               if (!*input || (unsigned char)*input > 0x7f || *input == 0xa || *input == 0xd) {
+                               ++input;
+                               if (!*input) {
+                                       break;
+                               }
+                               if ((unsigned char) *input > 0x7f || *input == 0xa || *input == 0xd) {
                                        continue;  /* not a valid quoted-pair, so skip it */
                                }
-                       } else if (((*input != 0x9) && ((unsigned char) *input < 0x20)) ||
-                                   (*input == 0x7f)) {
+                       } else if ((*input != 0x9 && (unsigned char) *input < 0x20)
+                               || *input == 0x7f) {
                                continue; /* skip this invalid character. */
                        }
 
-                       *output++ = *input;
-                       outputsize--;
+                       if (0 < outputsize) {
+                               /* We still have room for the output display-name. */
+                               *output++ = *input;
+                               --outputsize;
+                       }
                }
 
                /* if this is successful, input should be at the ending quote */
-               if (!input || *input != '"') {
+               if (*input != '"') {
                        ast_log(LOG_WARNING, "No ending quote for display-name was found\n");
                        *orig_output = '\0';
                        return orig_input;
                }
 
                /* make sure input is past the last quote */
-               input++;
+               ++input;
 
-               /* terminate outbuf */
+               /* terminate output */
                *output = '\0';
        } else {  /* either an addr-spec or tokenLWS-combo */
-               for (;((outputsize > 0) && *input); input++) {
+               for (; *input; ++input) {
                        /* token or WSP (without LWS) */
                        if ((*input >= '0' && *input <= '9') || (*input >= 'A' && *input <= 'Z')
                                || (*input >= 'a' && *input <= 'z') || *input == '-' || *input == '.'
                                || *input == '!' || *input == '%' || *input == '*' || *input == '_'
                                || *input == '+' || *input == '`' || *input == '\'' || *input == '~'
                                || *input == 0x9 || *input == ' ') {
-                               *output++ = *input;
-                               outputsize -= 1;
+                               if (0 < outputsize) {
+                                       /* We still have room for the output display-name. */
+                                       *output++ = *input;
+                                       --outputsize;
+                               }
                        } else if (*input == '<') {   /* end of tokenLWS-combo */
                                /* we could assert that the previous char is LWS, but we don't care */
                                break;
@@ -759,10 +773,10 @@ const char *get_calleridname(const char *input, char *output, size_t outputsize)
                        return orig_input;
                }
 
-               /* set NULL while trimming trailing whitespace */
+               /* terminate output while trimming any trailing whitespace */
                do {
                        *output-- = '\0';
-               } while (*output == 0x9 || *output == ' '); /* we won't go past orig_output as first was a non-space */
+               } while (orig_output <= output && (*output == 0x9 || *output == ' '));
        }
 
        return input;
@@ -771,11 +785,12 @@ const char *get_calleridname(const char *input, char *output, size_t outputsize)
 AST_TEST_DEFINE(get_calleridname_test)
 {
        int res = AST_TEST_PASS;
-       const char *in1 = "\" quoted-text internal \\\" quote \"<stuff>";
+       const char *in1 = " \" quoted-text internal \\\" quote \"<stuff>";
        const char *in2 = " token text with no quotes <stuff>";
        const char *overflow1 = " \"quoted-text overflow 1234567890123456789012345678901234567890\" <stuff>";
+       const char *overflow2 = " non-quoted text overflow 1234567890123456789012345678901234567890 <stuff>";
        const char *noendquote = " \"quoted-text no end <stuff>";
-       const char *addrspec = " \"sip:blah@blah <stuff>";
+       const char *addrspec = " sip:blah@blah";
        const char *no_quotes_no_brackets = "blah@blah";
        const char *after_dname;
        char dname[40];
@@ -810,11 +825,19 @@ AST_TEST_DEFINE(get_calleridname_test)
        /* quoted-text buffer overflow */
        after_dname = get_calleridname(overflow1, dname, sizeof(dname));
        ast_test_status_update(test, "overflow display-name1: %s\nafter: %s\n", dname, after_dname);
-       if (*dname != '\0' && after_dname != overflow1) {
+       if (strcmp(dname, "quoted-text overflow 123456789012345678")) {
                ast_test_status_update(test, "overflow display-name1 test failed\n");
                res = AST_TEST_FAIL;
        }
 
+       /* non-quoted-text buffer overflow */
+       after_dname = get_calleridname(overflow2, dname, sizeof(dname));
+       ast_test_status_update(test, "overflow display-name2: %s\nafter: %s\n", dname, after_dname);
+       if (strcmp(dname, "non-quoted text overflow 12345678901234")) {
+               ast_test_status_update(test, "overflow display-name2 test failed\n");
+               res = AST_TEST_FAIL;
+       }
+
        /* quoted-text buffer with no terminating end quote */
        after_dname = get_calleridname(noendquote, dname, sizeof(dname));
        ast_test_status_update(test, "noendquote display-name1: %s\nafter: %s\n", dname, after_dname);
@@ -825,7 +848,7 @@ AST_TEST_DEFINE(get_calleridname_test)
 
        /* addr-spec rather than display-name. */
        after_dname = get_calleridname(addrspec, dname, sizeof(dname));
-       ast_test_status_update(test, "noendquote display-name1: %s\nafter: %s\n", dname, after_dname);
+       ast_test_status_update(test, "addr-spec display-name1: %s\nafter: %s\n", dname, after_dname);
        if (*dname != '\0' && after_dname != addrspec) {
                ast_test_status_update(test, "detection of addr-spec failed\n");
                res = AST_TEST_FAIL;
@@ -839,14 +862,13 @@ AST_TEST_DEFINE(get_calleridname_test)
                res = AST_TEST_FAIL;
        }
 
-
        return res;
 }
 
 int get_name_and_number(const char *hdr, char **name, char **number)
 {
        char header[256];
-       char tmp_name[50] = { 0, };
+       char tmp_name[50];
        char *tmp_number = NULL;
        char *hostport = NULL;
        char *dummy = NULL;
@@ -1069,14 +1091,14 @@ char *get_in_brackets(char *tmp)
 AST_TEST_DEFINE(get_in_brackets_test)
 {
        int res = AST_TEST_PASS;
-       char *in_brackets = "<sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah>";
+       char in_brackets[] = "sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah";
        char no_name[] = "<sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah>";
        char quoted_string[] = "\"I'm a quote stri><ng\" <sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah>";
        char missing_end_quote[] = "\"I'm a quote string <sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah>";
        char name_no_quotes[] = "name not in quotes <sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah>";
        char no_end_bracket[] = "name not in quotes <sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah";
        char no_name_no_brackets[] = "sip:name@host";
-       char missing_start_bracket[] = "name not in quotes sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah>";
+       char missing_start_bracket[] = "sip:name:secret@host:port;transport=tcp?headers=testblah&headers2=blahblah>";
        char *uri = NULL;
 
        switch (cmd) {
@@ -1093,57 +1115,49 @@ AST_TEST_DEFINE(get_in_brackets_test)
        }
 
        /* Test 1, simple get in brackets */
-       if (!(uri = get_in_brackets(no_name)) || !(strcmp(uri, in_brackets))) {
-
-               ast_test_status_update(test, "Test 1, simple get in brackets failed.\n");
+       if (!(uri = get_in_brackets(no_name)) || strcmp(uri, in_brackets)) {
+               ast_test_status_update(test, "Test 1, simple get in brackets failed. %s\n", uri);
                res = AST_TEST_FAIL;
        }
 
        /* Test 2, starts with quoted string */
-       if (!(uri = get_in_brackets(quoted_string)) || !(strcmp(uri, in_brackets))) {
-
-               ast_test_status_update(test, "Test 2, get in brackets with quoted string in front failed.\n");
+       if (!(uri = get_in_brackets(quoted_string)) || strcmp(uri, in_brackets)) {
+               ast_test_status_update(test, "Test 2, get in brackets with quoted string in front failed. %s\n", uri);
                res = AST_TEST_FAIL;
        }
 
        /* Test 3, missing end quote */
-       if (!(uri = get_in_brackets(missing_end_quote)) || !(strcmp(uri, in_brackets))) {
-
-               ast_test_status_update(test, "Test 3, missing end quote failed.\n");
+       if (!(uri = get_in_brackets(missing_end_quote)) || !strcmp(uri, in_brackets)) {
+               ast_test_status_update(test, "Test 3, missing end quote failed. %s\n", uri);
                res = AST_TEST_FAIL;
        }
 
        /* Test 4, starts with a name not in quotes */
-       if (!(uri = get_in_brackets(name_no_quotes)) || !(strcmp(uri, in_brackets))) {
-
-               ast_test_status_update(test, "Test 4, passing name not in quotes failed.\n");
+       if (!(uri = get_in_brackets(name_no_quotes)) || strcmp(uri, in_brackets)) {
+               ast_test_status_update(test, "Test 4, passing name not in quotes failed. %s\n", uri);
                res = AST_TEST_FAIL;
        }
 
        /* Test 5, no end bracket, should just return everything after the first '<'  */
-       if (!(uri = get_in_brackets(no_end_bracket)) || !(strcmp(uri, in_brackets))) {
-
-               ast_test_status_update(test, "Test 5, no end bracket failed.\n");
+       if (!(uri = get_in_brackets(no_end_bracket)) || !strcmp(uri, in_brackets)) {
+               ast_test_status_update(test, "Test 5, no end bracket failed. %s\n", uri);
                res = AST_TEST_FAIL;
        }
 
        /* Test 6, NULL input  */
        if ((uri = get_in_brackets(NULL))) {
-
                ast_test_status_update(test, "Test 6, NULL input failed.\n");
                res = AST_TEST_FAIL;
        }
 
        /* Test 7, no name, and no brackets. */
-       if (!(uri = get_in_brackets(no_name_no_brackets)) || (strcmp(uri, "sip:name@host"))) {
-
+       if (!(uri = get_in_brackets(no_name_no_brackets)) || strcmp(uri, "sip:name@host")) {
                ast_test_status_update(test, "Test 7 failed. %s\n", uri);
                res = AST_TEST_FAIL;
        }
 
        /* Test 8, no start bracket, but with ending bracket. */
-       if (!(uri = get_in_brackets(missing_start_bracket)) || !(strcmp(uri, in_brackets))) {
-
+       if (!(uri = get_in_brackets(missing_start_bracket)) || strcmp(uri, in_brackets)) {
                ast_test_status_update(test, "Test 8 failed. %s\n", uri);
                res = AST_TEST_FAIL;
        }
@@ -1158,20 +1172,42 @@ int parse_name_andor_addr(char *uri, const char *scheme, char **name,
                          char **residue)
 {
        char buf[1024];
-       char **residue2=residue;
+       char **residue2 = residue;
+       char *orig_uri = uri;
        int ret;
+
+       buf[0] = '\0';
        if (name) {
-               get_calleridname(uri,buf,sizeof(buf));
-               *name = buf;
+               uri = (char *) get_calleridname(uri, buf, sizeof(buf));
        }
-       ret = get_in_brackets_full(uri,&uri,residue);
-       if (ret == 0) { /* uri is in brackets so do not treat unknown trailing uri parameters as potential messageheader parameters */
-               *residue = *residue + 1; /* step over the first semicolon so as per parse uri residue */
+       ret = get_in_brackets_full(uri, &uri, residue);
+       if (ret == 0) {
+               /*
+                * The uri is in brackets so do not treat unknown trailing uri
+                * parameters as potential message header parameters.
+                */
+               if (residue && **residue) {
+                       /* step over the first semicolon as per parse_uri_full residue */
+                       *residue = *residue + 1;
+               }
                residue2 = NULL;
        }
 
-       return parse_uri_full(uri, scheme, user, pass, hostport, params, headers,
-                             residue2);
+       if (name) {
+               if (buf[0]) {
+                       /*
+                        * There is always room at orig_uri for the display-name because
+                        * at least one character has always been removed.  A '"' or '<'
+                        * has been removed.
+                        */
+                       strcpy(orig_uri, buf);
+                       *name = orig_uri;
+               } else {
+                       *name = "";
+               }
+       }
+
+       return parse_uri_full(uri, scheme, user, pass, hostport, params, headers, residue2);
 }
 
 AST_TEST_DEFINE(parse_name_andor_addr_test)
@@ -1313,7 +1349,7 @@ AST_TEST_DEFINE(parse_name_andor_addr_test)
                name = user = pass = hostport = headers = residue = NULL;
                params.transport = params.user = params.method = params.ttl = params.maddr = NULL;
                params.lr = 0;
-       ast_copy_string(uri,testdataptr->uri,sizeof(uri));
+               ast_copy_string(uri,testdataptr->uri,sizeof(uri));
                if (parse_name_andor_addr(uri, "sip:,sips:",
                                          testdataptr->nameptr,
                                          testdataptr->userptr,
@@ -1330,17 +1366,17 @@ AST_TEST_DEFINE(parse_name_andor_addr_test)
                        ((testdataptr->residueptr) && strcmp(testdataptr->residue, residue)) ||
                        ((testdataptr->paramsptr) && strcmp(testdataptr->params.transport,params.transport)) ||
                        ((testdataptr->paramsptr) && strcmp(testdataptr->params.user,params.user))
-               ) {
-                               ast_test_status_update(test, "Sub-Test: %s,failed.\n", testdataptr->desc);
-                               res = AST_TEST_FAIL;
+                       ) {
+                       ast_test_status_update(test, "Sub-Test: %s,failed.\n", testdataptr->desc);
+                       res = AST_TEST_FAIL;
                }
        }
 
-
        return res;
 }
 
-int get_comma(char *in, char **out) {
+int get_comma(char *in, char **out)
+{
        char *c;
        char *parse = in;
        if (out) {
@@ -1376,35 +1412,35 @@ int get_comma(char *in, char **out) {
        return 1;
 }
 
-int parse_contact_header(char *contactheader, struct contactliststruct *contactlist) {
+int parse_contact_header(char *contactheader, struct contactliststruct *contactlist)
+{
        int res;
        int last;
        char *comma;
        char *residue;
        char *param;
        char *value;
-       struct contact *contact=NULL;
+       struct contact *split_contact = NULL;
 
        if (*contactheader == '*') {
                return 1;
        }
 
-       contact = malloc(sizeof(*contact));
-
-       AST_LIST_HEAD_SET_NOLOCK(contactlist, contact);
-       while ((last = get_comma(contactheader,&comma)) != -1) {
+       split_contact = ast_calloc(1, sizeof(*split_contact));
 
+       AST_LIST_HEAD_SET_NOLOCK(contactlist, split_contact);
+       while ((last = get_comma(contactheader, &comma)) != -1) {
                res = parse_name_andor_addr(contactheader, "sip:,sips:",
-                                           &contact->name, &contact->user,
-                                           &contact->pass, &contact->hostport,
-                                           &contact->params, &contact->headers,
-                                           &residue);
+                       &split_contact->name, &split_contact->user,
+                       &split_contact->pass, &split_contact->hostport,
+                       &split_contact->params, &split_contact->headers,
+                       &residue);
                if (res == -1) {
                        return res;
                }
 
                /* parse contact params */
-               contact->expires = contact->q = "";
+               split_contact->expires = split_contact->q = "";
 
                while ((value = strchr(residue,'='))) {
                        *value++ = '\0';
@@ -1417,20 +1453,19 @@ int parse_contact_header(char *contactheader, struct contactliststruct *contactl
                        }
 
                        if (!strcmp(param,"expires")) {
-                               contact->expires = value;
+                               split_contact->expires = value;
                        } else if (!strcmp(param,"q")) {
-                               contact->q = value;
+                               split_contact->q = value;
                        }
                }
 
-               if(last) {
+               if (last) {
                        return 0;
                }
                contactheader = comma;
 
-               contact = malloc(sizeof(*contact));
-               AST_LIST_INSERT_TAIL(contactlist, contact, list);
-
+               split_contact = ast_calloc(1, sizeof(*split_contact));
+               AST_LIST_INSERT_TAIL(contactlist, split_contact, list);
        }
        return last;
 }
@@ -1563,16 +1598,15 @@ AST_TEST_DEFINE(parse_contact_header_test)
                                        strcmp(tdcontactptr->params.transport, contactptr->params.transport) ||
                                        strcmp(tdcontactptr->params.ttl, contactptr->params.ttl) ||
                                        (tdcontactptr->params.lr != contactptr->params.lr)
-                               ) {
+                                       ) {
                                        ast_test_status_update(test, "Sub-Test: %s,failed.\n", testdataptr->desc);
                                        res = AST_TEST_FAIL;
                                        break;
                                }
 
-                       contactptr = AST_LIST_NEXT(contactptr,list);
+                               contactptr = AST_LIST_NEXT(contactptr,list);
                        }
                }
-
        }
 
        return res;