Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 37601: /trunk/gtk/ /trunk/gtk/: dcerpc_stat.c graph_anal

[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.