Bug 226294 – Use timingsafe_bcmp() in WTF::constantTimeMemcmp() when available

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Use timingsafe_bcmp() in WTF::constantTimeMemcmp() when available, instead of our own custom implementation.

[Chris Dumez]

Created attachment 429795 [details]
Patch

[Alex Christensen]

Comment on attachment 429795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429795&action=review

> Source/WTF/wtf/CryptographicUtilities.cpp:36
> +    return timingsafe_bcmp(voidA, voidB, length);

Why not timingsafe_memcmp, or should we rename constantTimeMemcmp to constantTimeBCmp?

[Chris Dumez]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 429795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429795&action=review
> 
> > Source/WTF/wtf/CryptographicUtilities.cpp:36
> > +    return timingsafe_bcmp(voidA, voidB, length);
> 
> Why not timingsafe_memcmp, or should we rename constantTimeMemcmp to
> constantTimeBCmp?

I didn't choose how the system API is called :) No, I cannot rename the system API to timingsafe_memcmp. We could rename the WTF:: constantTimeMemcmp to match BSD's naming but I personally prefer the memcmp naming.

[Alex Christensen]

Comment on attachment 429795 [details]
Patch

Oh, I was looking at the bsd documentation, which has both.
memcmp implies a directional return value that can be used in sorting, but this doesn't have that property.  Our use only checks whether it's zero, though, so it's fine so far.

[Alexey Proskuryakov]

Comment on attachment 429795 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429795&action=review

> Source/WTF/wtf/CryptographicUtilities.cpp:33
>  NEVER_INLINE int constantTimeMemcmp(const void* voidA, const void* voidB, size_t length)

This wrapper that's in a different dylib and forces no inlining is slightly wasteful when calling the system API. Perhaps implement the system API path in the header?

[EWS]

Committed r278140 (238185@main): <https://commits.webkit.org/238185@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429795 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78542643>

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 429795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429795&action=review
> 
> > Source/WTF/wtf/CryptographicUtilities.cpp:33
> >  NEVER_INLINE int constantTimeMemcmp(const void* voidA, const void* voidB, size_t length)
> 
> This wrapper that's in a different dylib and forces no inlining is slightly
> wasteful when calling the system API. Perhaps implement the system API path
> in the header?

Sure, I can do that. I actually had hesitated to do that. I'll follow-up.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 429821 [details]
Patch

[EWS]

Committed r278144 (238188@main): <https://commits.webkit.org/238188@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429821 [details].

[Sam Weinig]

This should probably use a new HAVE macro.

[Chris Dumez]

(In reply to Sam Weinig from comment #12)
> This should probably use a new HAVE macro.

It's there, don't you see it? http://trac.webkit.org/r278178 :D

[Sam Weinig]

(In reply to Chris Dumez from comment #13)
> (In reply to Sam Weinig from comment #12)
> > This should probably use a new HAVE macro.
> 
> It's there, don't you see it? http://trac.webkit.org/r278178 :D

Great!