Opened 6 weeks ago

Closed 5 weeks ago

#22870 closed defect (fixed)

consdiff test fails on OS X

Reported by: pastly Owned by:
Priority: Low Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by pastly)

See gitweb user/pastly/tor.git branch ticket22870

strcmp on most platforms returns -1 for "less than 0" and 1 for "more than 0" but +/- 1 isn't required. Today I learned that OS X is one of those platforms that follows the standard but not the convention.

Introduced in tor-0.3.1.1-alpha

Child Tickets

Change History (7)

comment:1 Changed 6 weeks ago by pastly

Description: modified (diff)
Status: newneeds_review

comment:2 Changed 6 weeks ago by ahf

Status: needs_reviewmerge_ready

Patch looks good to me.

comment:3 Changed 6 weeks ago by teor

Are there any other instances of this pattern in the codebase?
I just checked, we very rarely do anything other than:

!strcmp()
strcmp == 0
strcmp()

and all the instances where we do > or < seem ok.

What versions of macOS does this fail on?
(We should test on it more often.)

On macOS 10.12.5, Xcode 8.3.3, clang 3.9.1 I see:

consdiff/base64cmp: OK

But I also see in the macOS memcmp man page:

The memcmp() function returns zero if the two strings are identical, oth-
erwise returns the difference between the first two differing bytes
(treated as unsigned char values, so that \200' is greater than \0',
for example). Zero-length strings are always identical. This behavior
is not required by C and portable code should only depend on the sign of
the returned value.

Whereas strcmp is ambiguous:

The strcmp() and strncmp() functions return an integer greater than,
equal to, or less than 0, according as the string s1 is greater than,
equal to, or less than the string s2. The comparison is done using
unsigned characters, so that \200' is greater than \0'.

comment:4 in reply to:  3 Changed 6 weeks ago by pastly

Replying to teor:

What versions of macOS does this fail on?
(We should test on it more often.)

10.11.6 (I'm calling it OS X on purpose :p)

On macOS 10.12.5, Xcode 8.3.3, clang 3.9.1 I see:

consdiff/base64cmp: OK

But I also see in the macOS memcmp man page:

The memcmp() function returns zero if the two strings are identical, oth-
erwise returns the difference between the first two differing bytes
(treated as unsigned char values, so that \200' is greater than \0',
for example). Zero-length strings are always identical. This behavior
is not required by C and portable code should only depend on the sign of
the returned value.

Whereas strcmp is ambiguous:

The strcmp() and strncmp() functions return an integer greater than,
equal to, or less than 0, according as the string s1 is greater than,
equal to, or less than the string s2. The comparison is done using
unsigned characters, so that \200' is greater than \0'.

The behavior I saw was definitely more in line with what you've quoted for memcmp.

I'm very weak on OS X/macOS development. I don't seem to have Xcode installed. clang --version says

Apple LLVM version 8.0.0 (clang-800.0.42.1)
Target: x86_64-apple-darwin15.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
$ ./src/test/test --verbose consdiff/base..
consdiff/base64cmp:
         OK src/test/test_consdiff.c:402: assert(0 OP_EQ base64cmp_wrapper(NULL, NULL)): 0 vs 0
         OK src/test/test_consdiff.c:403: assert(-1 OP_EQ base64cmp_wrapper(NULL, "foo")): -1 vs -1
         OK src/test/test_consdiff.c:404: assert(1 OP_EQ base64cmp_wrapper("bar", NULL)): 1 vs 1
         OK src/test/test_consdiff.c:407: assert(0 OP_EQ base64cmp_wrapper("", "")): 0 vs 0
         OK src/test/test_consdiff.c:408: assert(0 OP_EQ base64cmp_wrapper("_", "&")): 0 vs 0
         OK src/test/test_consdiff.c:411: assert(0 OP_EQ base64cmp_wrapper("abcABC/+", "abcABC/+")): 0 vs 0
         OK src/test/test_consdiff.c:413: assert(0 OP_EQ base64cmp_wrapper("abcABC/+ ", "abcABC/+ ")): 0 vs 0
         OK src/test/test_consdiff.c:415: assert(-1 OP_EQ base64cmp_wrapper("abcABC/+ ", "abcABC/+a")): -1 vs -1
  FAIL src/test/test_consdiff.c:418: assert(-1 OP_EQ strcmp("/foo", "Afoo")): -1 vs -18
  [base64cmp FAILED]
1/1 TESTS FAILED. (0 skipped)
Last edited 6 weeks ago by pastly (previous) (diff)

comment:5 Changed 5 weeks ago by nickm

looks good; I have cherry-picked this to maint-0.3.1.

(The original branch was based on master, which would be right for a feature ... but a bugfix should be on the corresponding maint branch.)

comment:6 Changed 5 weeks ago by nickm

(I'm going to leave this open so people can look at the other memcmp/strcmp stuff for a while)

comment:7 Changed 5 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Closing this now; I've tried hunting for more cases with coccinelle, but I didn't find any.

Note: See TracTickets for help on using tickets.