lib/libc/string/bcmp.c: fix integer overflow bug

[fuz]

Summary

bcmp() returned the number of remaining bytes when the main loop exits.
In case of a match, this is zero, else a positive integer. On systems
where SIZE_MAX > INT_MAX, the implicit conversion from size_t to int in
the return value may cause the number of remaining bytes to overflow,
becoming zero and falsely indicating a successful comparison.

Fix the bug by always returning 0 on equality, 1 otherwise.
The new implementation returns different result than old one
within variation permitted by spec. May break compatibility.

Sponsored by: FreeBSD Foundation
PR: 272474 (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=272474)

Test Plan

run testsuite

[imp]

Otherwise this looks like the right fix.

lib/libc/string/bcmp.c

6

I don't thin this merits a copyright addition.

[mjg]

maybe whack this implementation to begin with and make it call memcmp, which *is* optimized on some archs

preferably this would just be an alias but some weird magic was getting in the way of accomodating that

[fuz]

@mjg You yourself wrote lib/libc/amd64/string/memcp.S which covers both memcmp and bcmp with some conditional code. Are you saying this approach was wrong and we should instead have bcmp call memcmp? The semantics of bcmp are indeed slightly faster to implement as the code doesn't have to find the specific bytes that mismatched and compute the difference.

lib/libc/string/bcmp.c

6

I have been instructed to put this copyright addition in by my contract with the FreeBSD Foundation. I found this bug during work on the contract and submitted the patch as part of it.

You may instead want to import the OpenBSD patch, which does basically the same thing.

[mjg]

I am saying a completely unpoptimized bcmp like this one should not exist and instead it would be better if it called memcmp and even better if it aliased it. That's not the same as being opposed to bcmp's existence as it can be faster than memcmp in case of a mismatch.

[imp]

lib/libc/string/bcmp.c

6

Your instructions are overly broad and no appropriate here. Remove this if you commit this. I think that your interpretation of their contract is in error here.

[jrm]

lib/libc/string/bcmp.c

6

@fuz, I believe you're following the contract text [1] as stated, but if @imp, who has been at this longer than any of us, thinks this doesn't warrant a copyright inclusion, let's follow his advice. Perhaps we need to tweak the text to say something like 'significantly modifies'? IANAL.

[1] License Obligations. Consultant shall ensure that the copyright notice and license text attached as Exhibit B
shall appear at the top of any new files that Consultant creates; and that the copyright notice and license
text attached as Exhibit C shall appear at the top of any pre-existing files that Consultant modifies during the
course of providing the Services.

[fuz]

@imp Are you okay if I commit with no copyright claim writing

Obtained from: OpenBSD: bcmp.c,v 1.9 2008/03/19 03:00:23 ray Exp

in the commit message? Referring to this bcmp.c diff.

[emaste]

lib/libc/string/bcmp.c

6

Yes something like "significantly modifies" would be appropriate for Foundation copyrights.

[emaste]

lib/libc/string/bcmp.c

6

And to be explicit: The FreeBSD Foundation approves this with the copyright header change excluded.

[fuz]

Remove foundation copyright header as per @emaste's instructions.

[emaste]

Note, should be Sponsored by: The FreeBSD Foundation

[imp]

I'm happy with this commit as landed.