[Privoxy-devel] filters.c: warning: 'xxx' may be used uninitialized in this function [-Wmaybe-uninitialized]

[Fabian Keil]

Lee <ler762 at gmail.com> wrote:

> before I go making changes to get rid of compiler warnings, is there
> really a problem with variables being uninitialized in filters.c?  I
> can't tell :(
>
> ----------- warnings
> i686-w64-mingw32-gcc -c -pipe -O2 -DWINVER=0x501   -mwindows -Wall
> -Ipcre  filters.c -o filters.o
> filters.c: In function 'match_sockaddr':
> filters.c:214:42: warning: 'address_port' may be used uninitialized in
> this function [-Wmaybe-uninitialized]
>     if (*netmask_port && *network_port != *address_port)
>                                           ^

What's the compiler version you're using?

I suspect that it gets confused by the return(-1) blocks in
sockaddr_storage_to_ip(). At first glance I believe that
they shouldn't be reachable in which case we should probably
remove them or make this more obvious.

We could start with something like:

commit 12b76b80bf0e41ab15c824d64e83beafb8d682fc
Author: Fabian Keil <fk at fabiankeil.de>
Date:   Thu Jun 1 18:56:25 2017 +0200

    Let sockaddr_storage_to_ip() assert that the address family is AF_INET or AF_INET6

diff --git a/filters.c b/filters.c
index 2ae03ded..7efc7e7b 100644
--- a/filters.c
+++ b/filters.c
@@ -113,6 +113,7 @@ static int sockaddr_storage_to_ip(const struct sockaddr_storage *addr,
                                   in_port_t **port)
 {
    assert(NULL != addr);
+   assert(addr->ss_family == AF_INET || addr->ss_family == AF_INET6);
 
    switch (addr->ss_family)
    {

commit 583336bdf0469ee041656ccd63190788fdc1a107
Author: Fabian Keil <fk at fabiankeil.de>
Date:   Thu Jun 1 18:51:52 2017 +0200

    Let sockaddr_storage_to_ip() assert that addr isn't NULL
    
    Calling the function with a NULL pointer is unreasonable.

diff --git a/filters.c b/filters.c
index 7f4c5b3e..2ae03ded 100644
--- a/filters.c
+++ b/filters.c
@@ -112,10 +112,7 @@ static int sockaddr_storage_to_ip(const struct sockaddr_storage *addr,
                                   uint8_t **ip, unsigned int *len,
                                   in_port_t **port)
 {
-   if (NULL == addr)
-   {
-      return(-1);
-   }
+   assert(NULL != addr);
 
    switch (addr->ss_family)
    {

And later on change sockaddr_storage_to_ip() to return void.

In three out of four cases the return code is currently ignored
anyway ...

> ------------------------ fix
> Index: current/filters.c
> ===================================================================
> RCS file: /cvsroot/ijbswa/current/filters.c,v
> retrieving revision 1.204
> diff -U5 -d -r1.204 filters.c
> --- current/filters.c   8 Mar 2017 13:13:18 -0000       1.204
> +++ current/filters.c   1 Jun 2017 13:55:45 -0000
> @@ -175,13 +175,13 @@
>   *********************************************************************/
>  static int match_sockaddr(const struct sockaddr_storage *network,
>                            const struct sockaddr_storage *netmask,
>                            const struct sockaddr_storage *address)
>  {
> -   uint8_t *network_addr, *netmask_addr, *address_addr;
> -   unsigned int addr_len;
> -   in_port_t *network_port, *netmask_port, *address_port;
> +   uint8_t *network_addr = NULL, *netmask_addr = NULL, *address_addr =
> NULL;
> +   unsigned int addr_len = 0;
> +   in_port_t *network_port = NULL, *netmask_port = NULL, *address_port
> = NULL; int i;

I would prefer to clarify the code first to see if this is sufficient
to convince your compiler that the variables are initialised properly.

Simply initialising the variables to 0/NULL is likely to result
in "dead store" warnings from other compilers.

If the stores weren't actually dead they wouldn't be sufficient to
prevent problems anyway.

Fabian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.privoxy.org/pipermail/privoxy-devel/attachments/20170601/5c837689/attachment.bin>