Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 37601: /trunk/gtk/ /trunk/gtk/: dcer

From: Дмитрий Дьяченко <dimhen@xxxxxxxxx>
Date: Wed, 8 Jun 2011 23:34:14 +0400
Look as "# if __USE_FORTIFY_LEVEL > 0" take place
For example, Fedora 15 /usr/include/sys/cdefs.h contains

/* If fortification mode, we warn about unused results of certain
   function calls which can lead to problems.  */
#if __GNUC_PREREQ (3,4)
# define __attribute_warn_unused_result__ \
   __attribute__ ((__warn_unused_result__))
# if __USE_FORTIFY_LEVEL > 0
#  define __wur __attribute_warn_unused_result__
# endif
#else
# define __attribute_warn_unused_result__ /* empty */
#endif
#ifndef __wur
# define __wur /* Ignore */
#endif

As i hear somewhere, Debian-based distros often set __USE_FORTIFY_LEVEL higher then others...

May be this is the root of the problem and possible way to resolve it?

Dmitry

2011/6/8 Guy Harris <guy@xxxxxxxxxxxx>

On Jun 8, 2011, at 7:46 AM, Дмитрий Дьяченко wrote:

> [FYI] http://gcc.gnu.org/ml/gcc/2010-05/msg00657.html
>
> [snip]
> As the compiler documentation states, warn_unused_result was intended
> for cases where failing to check the return value is always a security
> risk or a bug.  The documentation cites the example of realloc.  That
> is a case where casting the return value to (void) would always be
> wrong.

Yes, ignoring the value of malloc() or realloc() is always wrong - and, unless you're doing some form of testing where you're deliberately trying to eat up a ton of address space to make sure the right thing happens, completely pointless, because you're not using what you've allocated or reallocated.

However, if

       1) you're using strtoul() only to check whether a string is a valid number

and

       2) you clear errno before calling strtoul() and check it afterwards

it's perfectly proper and reasonable to ignore the return value of strtoul().

The problem is that the return value of strtoul() is *NOT* a success-or-failure indication - it's a "here's the result" or "I couldn't convert the value, but I had to return *something*, so I'm returning a value that could also be returned on success" indication.  For malloc()/realloc(), unless you're passing 0 as the size, it should never return a null pointer on success, so there's an out-of-band value that can be used to indicate an error; for strtoul(), there is no out-of-band value that can be used to indicate an error.

So, frankly, I'd say the bug is in glibc, or whatever code chose to mark strtoul() as a "warn_unused_result" function.  Checking the return value is neither necessary nor sufficient to detect errors, so it is *not* always a security risk or a bug not to check the return value of strtoul(

>  The compiler really should warn for that code by default; if
> you have some crazy need to ignore the result of realloc, just use the
> -Wno-unused-result option.

Unfortunately, -Wno-unused-result takes no argument to tell it which *particular* functions should not be warned about, so it's a bit of a big hammer to use.

The message you quote continues:

> [stuff about the immediate problem being discussed]
>
> So what are the right choices here?  I tend to be reluctant to endorse
> adding a new option, but I can't think of another approach.  I think
> we should consider introducing a new gcc function attribute:
> must_use_result.  I think we should document that attribute as
> intended specifically for cases where failing to use the return value
> is a program error, as with calls to realloc.  We should handle
> must_use_result and warn_unused_result similarly, except that adding a
> cast to (void) disables the warn_unused_result warning.  Perhaps there
> should also be other simple ways to disable the warn_unused_result
> warning.

I'd vote either for

       1) Ian Lance Taylor's quoted proposal, along with *NOT* marking strtoull() as must_use_result

or

       2) not marking strtoull() as warn_unused_result, given that ignoring its return value is not always a security risk or a bug

as the "right" fix to that particular problem.

As for Wireshark's problem, perhaps either using spin buttons for numeric preferences, or otherwise making it impossible to type something that's not a number into the GUI for those preferences, and thus avoiding the need to check whether it's a valid number, would also be a good idea.
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
            mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe