Am 2023-09-10 18:53, schrieb Robert Clausecker:
Hi Warner,Thank you for your response. Am Sun, Sep 10, 2023 at 09:53:03AM -0600 schrieb Warner Losh:On Sun, Sep 10, 2023, 7:36 AM Robert Clausecker <f...@fuz.su> wrote: > Hi Warner, > > I have pushed a fix. It should hopefully address those failing tests. > The same issue should also affect memcmp(), but unlike for memchr(), it is > illegal to pass a length to memcmp() that extends past the actual end of > the buffer as memcmp() is permitted to examine the whole buffer regardless > of where the first mismatch is. > > I am considering a change to improve the behaviour of memcmp() on such > errorneous inputs. There are two options: (a) I could change memcmp() the > same way I fixed memchr() and have implausible buffer lengths behave as if > the buffer goes to the end of the address space or (b) I could change > memcmp() to crash loudly if it detects such a case. I could also > (c) leave memcmp() as is. Which of these three choices is preferable? >What does the standard say? I'm highly skeptical that these corner cases areUB behavior.I'd like actual support for this statement, rather than your conjecturethat it'sillegal. Even if you can come up with that, preserving the old behavior ismyfirst choice. Especially since many of these functions aren't well definedby a standard, but are extensions. As for memchr, https://pubs.opengroup.org/onlinepubs/009696799/functions/memchr.html has no such permission to examine 'the entire buffer at once' nor any restirctionas to the length extending beyond the address space. I'm skeptical of yourreadingthat it allows one to examine all of [b, b + len), so please explain wherethe standard supports reading past the first occurance.memchr() in particular is specified to only examine the input until the matching character is found (ISO/IEC 9899:2011 § 7.24.5.1): *** The memchr function locates the first occurrence of c (converted to anunsigned char) in the initial n characters (each interpreted as unsignedchar) of the object pointed to by s. The implementation shall behave as if it reads the characters sequentially and stops as soon as a matching character is found. *** Therefore, it appears reasonable that calls with fake buffer lengths (e.g. SIZE_MAX, to read until a mismatch occurs) must be supported. However, memcmp() has no such language and the text explicitly states that the whole buffer is compared (ISO/IEC 9899:2011 § 7.24.4.1): *** The memcmp function compares the first n characters of the objectpointed to by s1 to the first n characters of the object pointed to by s2.*** By omission, this seems to give license to e.g. implement memcmp() like timingsafe_memcmp() where it inspects all n characters of both buffers and only then gives a result. So if n is longer than the actual buffer (e.g. n == SIZE_MAX), behaviour may not be defined (e.g. there could be a crash due to crossing into an unmapped page).Thus I have patched memchr() to behave correctly when length SIZE_MAX isgiven (commit b2618b65). My memcmp() suffers from similarly flawedlogic and may need to be patched. However, as the language I cited abovedoes not indicate that such usage needs to be supported for memcmp() (whereas it must be for memchr(), contrary to my assumptions), I was asking you for how to proceed with memcmp (hence choices (a)--(c)).
My 2ct: What did the previous implementation of memcmp() do in this case? - If it was generous and behaved similar to the requirements of memchr(), POLA requires to have the same now too. - If it was crashing or silently going on (= lurking bugs in 3rd party code), we may have the possibility to do a coredump in case of running past the end of the buffer to prevent malicous use. - In general I go with the robustness principle, "be liberal what you accept, but strict in what you provide" = memcmp() should behave as if it is supported. Bye, Alexander. -- http://www.Leidinger.net alexan...@leidinger.net: PGP 0x8F31830F9F2772BF http://www.FreeBSD.org netch...@freebsd.org : PGP 0x8F31830F9F2772BF
signature.asc
Description: OpenPGP digital signature