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-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 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. 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. Thanks, Tamar > > Thanks, > Kyrill > > > > > Thanks, > > Tamar > > > > gcc/ChangeLog: > > > > * config/aarch64/driver-aarch64.c (readline): Check return value > > fgets. > > > > --
