Thank you for the code review, Robert.
I have moved the buffer length check first, so I no longer
need the second 'has_prefix_len'.
I limit the 'atoi' replacement to 3 digits. I have also added
a check for 'isascii', as is done four lines earlier.
I have restored the newline that removing 'puts' removed.
Good catch.
> personally I'd question using any strcmp() type function for checking
> just two chars:
Using strncmp seems more readable because the search string
appears together as "+ ", and it also looks more like the old code.
(But I see your point, and if you feel strongly about it,
I'm also happy to substitute your rewrite.)
Here is patch version 2:
diff --git a/mts/smtp/smtp.c b/mts/smtp/smtp.c
index 00efde50..e6b34e08 100644
--- a/mts/smtp/smtp.c
+++ b/mts/smtp/smtp.c
@@ -819,14 +819,15 @@ again: ;
for (more = false; (buffer = netsec_readline(nsc, &buflen,
&errstr)) != NULL ; ) {
if (doingEHLO
+ && buflen > 4
&& has_prefix_len(buffer, buflen, "250")
- && (buffer[3] == '-' || doingEHLO == 2)
- && buffer[4]) {
+ && (buffer[3] == '-' || doingEHLO == 2)) {
if (doingEHLO == 2) {
- if ((*ehlo = malloc ((size_t) (strlen (buffer + 4) + 1)))) {
- strcpy (*ehlo++, buffer + 4);
+ if ((*ehlo = malloc (buflen - 4 + 1))) {
+ memcpy (*ehlo, buffer + 4, buflen - 4);
+ *(*ehlo++ + buflen - 4) = '\0';
*ehlo = NULL;
if (ehlo >= EHLOkeys + MAXEHLO)
doingEHLO = 0;
} else
@@ -840,10 +841,14 @@ again: ;
for (; buflen > 0 && (!isascii (*bp) || !isdigit (*bp)); bp++, buflen--)
continue;
cont = false;
- code = atoi ((char *) bp);
- bp += 3, buflen -= 3;
+ size_t len_limit = buflen < 3 ? 0 : buflen - 3;
+ for (code = 0;
+ buflen > len_limit && isascii (*bp) && isdigit (*bp);
+ bp++, buflen--) {
+ code = code * 10 + (*bp - '0');
+ }
for (; buflen > 0 && isspace (*bp); bp++, buflen--)
continue;
if (buflen > 0 && *bp == '-') {
cont = true;
@@ -879,9 +884,9 @@ again: ;
if (more)
continue;
if (sm_reply.code < 100) {
if (sm_verbose) {
- puts(sm_reply.text);
+ printf("%.*s\n", sm_reply.length, sm_reply.text);
fflush (stdout);
}
goto again;
}
@@ -1046,9 +1051,10 @@ sm_sasl_callback(enum sasl_message_type mtype, unsigned const char *indata,
return NOTOK;
}
if (!has_prefix_len(line, len, "334 ")) {
- netsec_err(errstr, "Improper SASL protocol response: %s", line);
+ netsec_err(errstr, "Improper SASL protocol response: %.*s",
+ (int) len, line);
return NOTOK;
}
if (len == 4) {
@@ -1098,11 +1104,13 @@ sm_sasl_callback(enum sasl_message_type mtype, unsigned const char *indata,
return NOTOK;
if (!has_prefix_len(line, len, "235 ")) {
if (len > 4)
- netsec_err(errstr, "Authentication failed: %s", line + 4);
+ netsec_err(errstr, "Authentication failed: %.*s",
+ (int) len - 4, line + 4);
else
- netsec_err(errstr, "Authentication failed: %s", line);
+ netsec_err(errstr, "Authentication failed: %.*s",
+ (int) len, line);
return NOTOK;
}
break;
diff --git a/uip/popsbr.c b/uip/popsbr.c
index d0059cb2..86fe019e 100644
--- a/uip/popsbr.c
+++ b/uip/popsbr.c
@@ -391,9 +391,9 @@ pop_sasl_callback(enum sasl_message_type mtype, unsigned const char *indata,
/*
* If the protocol is being followed correctly, should just
* be a "+ ", nothing else.
*/
- if (len != 2 || strcmp(line, "+ ") != 0) {
+ if (len != 2 || strncmp(line, "+ ", 2) != 0) {
netsec_err(errstr, "Did not get expected blank response "
"for initial challenge response");
return NOTOK;
}
@@ -446,9 +446,9 @@ pop_sasl_callback(enum sasl_message_type mtype, unsigned const char *indata,
* If we get an -ERR reply, bubble that back up
*/
if (has_prefix_len(line, len, "-ERR")) {
- netsec_err(errstr, "%s", line);
+ netsec_err(errstr, "%.*s", (int) len, line);
return NOTOK;
}
/*
@@ -508,9 +508,9 @@ pop_sasl_callback(enum sasl_message_type mtype, unsigned const char *indata,
if (line == NULL)
return NOTOK;
if (!has_prefix_len(line, len, "+OK")) {
- netsec_err(errstr, "Authentication failed: %s", line);
+ netsec_err(errstr, "Authentication failed: %.*s", (int) len, line);
return NOTOK;
}
break;
