httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <trawi...@bellsouth.net>
Subject Re: [PATCH] Warning clean-ups....
Date Thu, 21 Sep 2000 14:25:01 GMT
"Victor J. Orlikowski" <v.j.orlikowski@gte.net> writes:

> Index: src/main/http_protocol.c
> ===================================================================
> RCS file: /cvs/apache/apache-2.0/src/main/http_protocol.c,v
> retrieving revision 1.126
> diff -u -d -b -r1.126 http_protocol.c
> --- http_protocol.c	2000/09/14 20:43:14	1.126
> +++ http_protocol.c	2000/09/15 15:54:21
> @@ -1711,7 +1711,7 @@
>  {
>      int methnum;
>      int i;
> -    char **methods;
> +    char **methods = NULL;

This is not a good change as far as I can tell.  The use of methods in
this function is FUBAR and this doesn't help.  It is better to get a
warning at compile time (until somebody wishes to initialize methods
to the proper array) than to segfault upon execution of an obscure
path. 

Ken... Isn't this your recent addition?  What should methods be set to
prior to accessing it in the loop in the heart of the function?

>  
>      /*
>       * If it's one of our known methods, use the shortcut and check the
> @@ -1743,7 +1743,7 @@
>      int methnum;
>      int i;
>      const char **xmethod;
> -    char **methods;
> +    char **methods = NULL;

ditto

>  
>      /*
>       * If it's one of our known methods, use the shortcut and use the
> @@ -1775,7 +1775,7 @@
>  				       const char *method)
>  {
>      int methnum;
> -    char **methods;
> +    char **methods = NULL;

ditto

>  
>      /*
>       * If it's one of our known methods, use the shortcut and use the
> Index: src/modules/standard/mod_include.c
> ===================================================================
> RCS file: /cvs/apache/apache-2.0/src/modules/standard/mod_include.c,v
> retrieving revision 1.56
> diff -u -d -b -r1.56 mod_include.c
> --- mod_include.c	2000/09/14 18:42:56	1.56
> +++ mod_include.c	2000/09/15 15:54:23
> @@ -338,7 +338,7 @@
>  static char *get_tag(apr_pool_t *p, ap_bucket *in, char *tag, int tagbuf_len, int dodecode,
apr_off_t *offset)
>  {
>      ap_bucket *dptr = in;
> -    const char *c;
> +    const char *c = NULL;

This function looks uncool... It *deserves* a warning :)  (No, I'm
just too lazy to look at it in sufficient detail right now.)

Following the thread of variable c to see if it is really used before
being set points out some other issues to research, which probably
means it is worthwhile to get a warning.  Setting c to NULL wouldn't
resolve any issues but would make them less noticeable.

Some of the issues:

  the while condition on the first loop looks bad; it should perhaps
  be "while (1) {" or "while (dptr != sentinel-value) {".  "while
  (dptr)" acts the same as "while (1)" but is confusing.

  what happens if there is a zero-len bucket somewhere?  we don't
  check for that after dptr->read(); maybe we are guaranteed not to
  get such a bucket in this function?  with Ryan's changes to the
  split routines, we can get zero-length buckets in more situations

(Ryan: how much of mod_include were you planning to rewrite, anyway?
More than the main parsing loop which creates a brigade with the tag?)

>      const char *str;
>      apr_ssize_t length; 
>      char *t = tag, *tag_val, term;
> @@ -2267,7 +2267,7 @@
>      int conditional_status;
>      ap_bucket *dptr = AP_BRIGADE_FIRST(bb);
>      ap_bucket *tagbuck, *dptr2;
> -    ap_bucket *endsec;
> +    ap_bucket *endsec = NULL;

I think that a change Ryan agreed to this weekend would take care of this
warning (look for "wet noodle" and related thread in the archives :) ).
I think that Ryan is going to rewrite this code soon, but if that
other change doesn't hurt and it gets rid of the warning I'll put it in.

-- 
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
     http://www.geocities.com/SiliconValley/Park/9289/
          Born in Roswell... married an alien...

Mime
View raw message