Suggested replacement of ap_snprintf with the native alternative snprintf with error check == Patch attached ==

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Suggested replacement of ap_snprintf with the native alternative snprintf with error check == Patch attached ==

Daniel Ouellet
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)) {

Reply | Threaded
Open this post in threaded view
|

Re: Suggested replacement of ap_snprintf with the native alternative snprintf with error check == Patch attached ==

Ted Unangst-2
On 3/28/06, Daniel Ouellet <[hidden email]> wrote:
> +    l = snprintf(errstr + len, sizeof(errstr) - len,
> +       "[%s] ", priorities[level & APLOG_LEVELMASK].t_name);
> +    if (l == -1 || l >= (int)sizeof(errstr))

the check for overflow is incorrect.  the size parameter is
sizeof(errstr) - len, that is what you should check against.

Reply | Threaded
Open this post in threaded view
|

Re: Suggested replacement of ap_snprintf with the native alternative snprintf with error check == Patch attached ==

Daniel Ouellet
Ted Unangst wrote:
> On 3/28/06, Daniel Ouellet <[hidden email]> wrote:
>> +    l = snprintf(errstr + len, sizeof(errstr) - len,
>> +       "[%s] ", priorities[level & APLOG_LEVELMASK].t_name);
>> +    if (l == -1 || l >= (int)sizeof(errstr))
>
> the check for overflow is incorrect.  the size parameter is
> sizeof(errstr) - len, that is what you should check against.

Shoot me! That was stupid from me. Need to get use to these new glasses
of mine.

Thanks for taking the time to check it out?

I will redo it. Any other comments? I am still unsure the best OpenBSD
approach for fail case here, return without changes assuming the calling
side will check for errors, not likely all the time, or simply use errx
type and exit?

I appreciate your feedback and time into this already!

Thanks Ted!!