Opened 7 years ago

Closed 7 years ago

#9066 closed defect (fixed)

use a macro like CHECK_PRINTF for tor_sscanf

Reported by: x3j11 Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.12-alpha
Severity: Keywords: tor-client testing
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

On OpenBSD/5.2 under gcc 4.2.1, the test case src/test/test_util.c warns on compile with

src/test/test_util.c: In function 'test_util_sscanf':
src/test/test_util.c:1570: warning: Array size (20) smaller than format string size (1000000) (arg 3)
src/test/test_util.c:1675: warning: Array size (20) smaller than format string size (1000) (arg 3)

This is an extra warning applied by OpenBSD for the platform's default compiler (see also ticket 7260 where this warning was also raised). I believe the cases used in the test are safe, so, to address this, let's start by removing the check for GNUC and instead use a macro like the existing CHECK_PRINTF macro, so we can condition it appropriately with a later patch.

Child Tickets

Attachments (1)

0001-Instead-of-testing-for-__GNUC__-use-CHECK_SCANF-like.patch (1.9 KB) - added by x3j11 7 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 7 years ago by x3j11

Status: newneeds_review
Strategem: add checks to configure.ac to flag if the CC we're using supports the format attribute first of all (defining CC_FORMAT_ATTRIBUTE), and another if it triggers this warning (defining something like CC_FORMAT_ATTRIBUTE_STRICT). Then define something like DISABLE_FORMAT_STRICT in the affected test. Then, we have CHECK_SCANF is empty if !CC_FORMAT_ATTRIBUTE
(CC_FORMAT_ATTRIBUTE_STRICT && DISABLE_FORMAT_STRICT).

comment:2 Changed 7 years ago by x3j11

Component: - Select a componentTor

comment:3 Changed 7 years ago by nickm

Status: needs_reviewnew

I agree with this one change, though I'm less confident that the strategy above is the right way to handle the openbsd warning issue.

The warning in this case is indeed correct: the unit test is trying to do something questionable there. I thought we'd suppressed that (see discussion on #7260), but if not, we should actually fix it so that the C isn't doing anything iffy IMO. After all, we don't want to disable scanf checking for the whole program just because the compiler can catch *more* errors!

Applying the patch above to 0.2.5

comment:4 in reply to:  3 Changed 7 years ago by andrea

Replying to nickm:

I agree with this one change, though I'm less confident that the strategy above is the right way to handle the openbsd warning issue.

The warning in this case is indeed correct: the unit test is trying to do something questionable there. I thought we'd suppressed that (see discussion on #7260), but if not, we should actually fix it so that the C isn't doing anything iffy IMO. After all, we don't want to disable scanf checking for the whole program just because the compiler can catch *more* errors!

Applying the patch above to 0.2.5

Let's consider the two proposed tests separately; regardless of the merits of shutting off the warnings in some cases, it's worth actually checking for format(printf,...)_ and format(scanf,...) rather than assuming #ifdef GNU_C is sufficient - presumably sufficiently old gccs don't have those extensions, and they're useful enough that some non-gcc compiler might implement compatible ones - either now without my being aware of it, or in the future.

comment:5 Changed 7 years ago by andrea

The particular test in question appears to be an OpenBSD-specific extension; an examination of gcc/c-family/c-format.c in the latest gcc-4.8.1, where the attribute(format()) warnings are implemented, shows no sign of checking string argument widths, and grepping the source for 'format string size' turns up negative.

I'll also note that in the particular instance reported in test_util.c, the function actually is provably safe, since the string being sscanf()ed is shorter than the buffer even though the field precision in the format string is longer. This is a case of a modified gcc implementing a stricter test than standard gcc, but not precisely enough to notice that the particular instance is safe.

I do not believe it would be possible for a compiler to support doing this test in a way that would recognize this, though, since attribute(format(scanf,...)) only tells the compiler which is the format string and where the formatted arg sequence starts. There's no way for it know this is an sscanf()-alike more specifically and which is the input string, and reason from there even if the input string is a constant which would make it possible to draw that conclusion if it did know.

comment:6 Changed 7 years ago by nickm

presumably sufficiently old gccs don't have those extensions, and they're useful enough that some non-gcc compiler might implement compatible ones - either now without my being aware of it, or in the future.

They were introduced in gcc 2.0, which was released in 1992 -- we will never want to build with a GCC that old, I think. (I tried to bootstrap gcc 2.0 for the fun of it, but it doesn't even build cleanly any more.)

There are also compilers that support this extension: notably, clang and (I believe) icc. But they both define GNUC to indicate that they support the usual GCC extensions.

I'll consider the other points when I'm more awake, but I wanted to braindump the above stuff before I forgot it.

comment:7 Changed 7 years ago by nickm

Keywords: tor-client testing added

comment:8 Changed 7 years ago by nickm

Resolution: fixed
Status: newclosed

Okay, I believe we were right to merge that patch, and we've taken this one about as far as we can.

Note: See TracTickets for help on using tickets.