[GitHub] astitcher commented on issue #176: NO-JIRA: [c] Fix Coverity warning of buffer overrun in pn_proactor_addr
astitcher commented on issue #176: NO-JIRA: [c] Fix Coverity warning of buffer overrun in pn_proactor_addr URL: https://github.com/apache/qpid-proton/pull/176#issuecomment-465784333 How about something like (compiles and passes tests with valgrind, but not tested with sanitisers or coverity): ``` int pn_proactor_addr(char *buf, size_t len, const char *host, const char *port) { /* Don't use snprintf, Windows is not C99 compliant and snprintf is broken. */ size_t hostlen = host ? strlen(host) : 0; size_t portlen = port ? strlen(port) : 0; if (buf && len > 0) { if (host) { strncpy(buf, host, len); } else { buf[0] = '\0'; } if (hostlen+2 < len) { buf[hostlen] = ':'; buf[hostlen+1] = '\0'; if (port) strncat(buf, port, len-hostlen-1); } buf[len-1] = '\0'; } return hostlen + portlen + 1; } ``` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] astitcher commented on issue #176: NO-JIRA: [c] Fix Coverity warning of buffer overrun in pn_proactor_addr
astitcher commented on issue #176: NO-JIRA: [c] Fix Coverity warning of buffer overrun in pn_proactor_addr URL: https://github.com/apache/qpid-proton/pull/176#issuecomment-465646443 Thinking more... > I don't think this is the correct fix. There is no problem using strncat that requires inventing a new (and possibly wrong) alternative that does the same thing but perhaps a bit different. > This is still my view. Rereading the code and man pages the code is incorrect, but introducing a new strncat isn't necessary. Just reduce the len parameter to strncat by the size of the copied host and ':' chars to avoid the overrun. You then only need the do strlen on the host. Also doing strncat(host); strncat(':'); strncat(port) is wrong. The output buffer should start with strncpy() as we don't want to append to anything already in the buffer but overwrite it (as documented in the header file) > In any case as far as I can see there isn't a buffer overrun problem with the fixed code at all! pn_proactor_addr can only overrun the buffer it's given if you give it the wrong length. This is just wrong - sorry! This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] astitcher commented on issue #176: NO-JIRA: [c] Fix Coverity warning of buffer overrun in pn_proactor_addr
astitcher commented on issue #176: NO-JIRA: [c] Fix Coverity warning of buffer overrun in pn_proactor_addr URL: https://github.com/apache/qpid-proton/pull/176#issuecomment-465636973 I don't think this is the correct fix. There is no problem using strncat that requires inventing a new (and possibly wrong) alternative that does the same thing but perhaps a bit different. In any case as far as I can see there isn't a buffer overrun problem with the fixed code at all! pn_proactor_addr can only overrun the buffer it's given if you give it the wrong length. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org