SEO

I would appreciate feedback on this suggested patch. I have done all the
ap_snprintf() replacement in my local code, but before sending the diff,
I would very much appreciate comments on how it is done below. Find one
sample only for http_log.c as an example.

I follow the man page for the snprintf and look how it was done in a few
example in the tree.

If errors are detected, I stop processing and return then like the man
suggested.

But some function where this is use have void return, pointers, etc. I
return accordingly, but I am not sure it is right to assume the calling
function will check for proper returns. It's not like everything can be
corrected at once. So, assuming the calling function will process
properly the return value is the right way to do it in here in
correcting the code? But what about when it is not? I guess it can't be
worst then it is already now as is? Is it OK to do so?

Also, when one would actually decide that it's should exit instead and
may be put feedback in the log file. I did this only in main/buff.c, but
I do question my choice however. I think it was more damaging there, but
may be it is the wrong thinking here on my part?

Feedback welcome.

Thanks for your time.

Daniel

++++++++++++++++++++++++++++


Index: http_log.c
================================================== =================
RCS file: /cvs/src/usr.sbin/httpd/src/main/http_log.c,v
retrieving revision 1.17
diff -u -r1.17 http_log.c
--- http_log.c 9 Feb 2005 12:13:09 -0000 1.17
+++ http_log.c 29 Mar 2006 07:17:51 -0000
@@ -238,7 +238,7 @@
char errstr[MAX_STRING_LEN];
char scratch[MAX_STRING_LEN];
size_t len;
- int save_errno = errno;
+ int l, save_errno = errno;
FILE *logf;

if (s == NULL) {
@@ -273,17 +273,27 @@
}

if (logf) {
- len = ap_snprintf(errstr, sizeof(errstr), "[%s] ", ap_get_time());
- } else {
+ l = snprintf(errstr, sizeof(errstr), "[%s] ", ap_get_time());
+ if (l == -1 || l >= (int)sizeof(errstr))
+ return;
+ else
+ len = l;
+ } else
len = 0;
- }

- len += ap_snprintf(errstr + len, sizeof(errstr) - len,
- "[%s] ", priorities[level & APLOG_LEVELMASK].t_name);
+ l = snprintf(errstr + len, sizeof(errstr) - len,
+ "[%s] ", priorities[level & APLOG_LEVELMASK].t_name);
+ if (l == -1 || l >= (int)sizeof(errstr))
+ return;
+ else
+ len += l;

if (file && (level & APLOG_LEVELMASK) == APLOG_DEBUG) {
- len += ap_snprintf(errstr + len, sizeof(errstr) - len,
- "%s(%d): ", file, line);
+ l = snprintf(errstr + len, sizeof(errstr) - len, "%s(%d): ", file, line);
+ if (l == -1 || l >= (int)sizeof(errstr))
+ return;
+ else
+ len += l;
}
if (r) {
/* XXX: TODO: add a method of selecting whether logged client
@@ -291,14 +301,22 @@
* quad is the most secure, which is why I'm implementing it
* first. -djg
*/
- len += ap_snprintf(errstr + len, sizeof(errstr) - len,
- "[client %s] ", r->connection->remote_ip);
+ l = snprintf(errstr + len, sizeof(errstr) - len,
+ "[client %s] ", r->connection->remote_ip);
+ if (l == -1 || l >= (int)sizeof(errstr))
+ return;
+ else
+ len += l;
}
if (!(level & APLOG_NOERRNO)
&& (save_errno != 0)
) {
- len += ap_snprintf(errstr + len, sizeof(errstr) - len,
- "(%d)%s: ", save_errno, strerror(save_errno));
+ l = snprintf(errstr + len, sizeof(errstr) - len,
+ "(%d)%s: ", save_errno, strerror(save_errno));
+ if (l == -1 || l >= (int)sizeof(errstr))
+ return;
+ else
+ len += l;
}

if (ap_vsnprintf(scratch, sizeof(scratch) - len, fmt, args)) {