Hi Tamar, > -----Original Message----- > From: Tamar Christina <tamar.christ...@arm.com> > Sent: 31 July 2020 09:07 > To: Kyrylo Tkachov <kyrylo.tkac...@arm.com>; gcc-patches@gcc.gnu.org > Cc: nd <n...@arm.com>; Richard Earnshaw <richard.earns...@arm.com>; > Marcus Shawcroft <marcus.shawcr...@arm.com>; Richard Sandiford > <richard.sandif...@arm.com> > Subject: RE: [PATCH] AArch64: Fix hwasan failure in readline. > > Hi Kyrill, > > > -----Original Message----- > > From: Kyrylo Tkachov <kyrylo.tkac...@arm.com> > > Sent: Friday, July 31, 2020 8:47 AM > > To: Tamar Christina <tamar.christ...@arm.com>; gcc- > patc...@gcc.gnu.org > > Cc: nd <n...@arm.com>; Richard Earnshaw <richard.earns...@arm.com>; > > Marcus Shawcroft <marcus.shawcr...@arm.com>; Richard Sandiford > > <richard.sandif...@arm.com> > > Subject: RE: [PATCH] AArch64: Fix hwasan failure in readline. > > > > Hi Tamar > > > > > -----Original Message----- > > > From: Tamar Christina <tamar.christ...@arm.com> > > > Sent: 30 July 2020 09:28 > > > To: gcc-patches@gcc.gnu.org > > > Cc: nd <n...@arm.com>; Richard Earnshaw > <richard.earns...@arm.com>; > > > Marcus Shawcroft <marcus.shawcr...@arm.com>; Kyrylo Tkachov > > > <kyrylo.tkac...@arm.com>; Richard Sandiford > > > <richard.sandif...@arm.com> > > > Subject: [PATCH] AArch64: Fix hwasan failure in readline. > > > > > > Hi All, > > > > > > My previous fix added an unchecked call to fgets in the new function > > readline. > > > fgets can fail when there's an error reading the file in which case it > > > returns NULL. It also returns NULL when the next character is EOF. > > > > > > The EOF case is already covered by the existing code but the error case > isn't. > > > This fixes it by returning the empty string on error. > > > > > > Also I now use strnlen instead of strlen to make sure we never read > > > outside the buffer. > > > > > > This was flagged by Matthew Malcomson during his hwasan work. > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > > > > > Ok for master? And for backport with the other patches? (haven't done > > > backport yet.) > > > > Code looks ok, but I'm wondering what kind of input triggered this. Now > that > > we can exercise this code in the testsuite (thanks!) perhaps a new test is > > in > > order? > > No code triggered this, this was HWAsan saying that there is a code-path > that can > potentially cause a failure. When the system call fails (i.e. corrupt file, > file > becomes > unavailable after you opened the stream etc) then the fgets call returns NULL, > but crucially the buffer is left in an indeterminate state. > > This means that the call to strlen can fail due to the buffer not having a \0 > in > it > and so you will read out of bounds in buf[last]. > > On it's own, the strlen -> strnlen change already covers this, but it would > return a > partial string, which is not what we want on failure. On failure we want to > abort > and so returning the empty string makes it ignore the entire block.
Ah, ok. > > If you prefer we can also hard abort. But I don't know how to emulate this > behaviour > In the testsuite. I would need to be able to asynchronously make the file > invalid after > the fopen call but during readline. > No need, we should just bail out here. This patch is okay. Thanks, Kyrill > Thanks, > Tamar > > > > > Thanks, > > Kyrill > > > > > > > > Thanks, > > > Tamar > > > > > > gcc/ChangeLog: > > > > > > * config/aarch64/driver-aarch64.c (readline): Check return value > > > fgets. > > > > > > --
