Re: [PATCH] [IOS] Avoid spurious EOF when writing partial bytes

[Jose E. Marchesi]

I forgot to mention a corollary of this bug.

In poke sometimes writing requires reading, like for example when
writing weird integers or unaligned integers, i.e. when working with
partial bytes, which should be "completed" with their current value in
the IO space.

Now, we support write-only IO spaces.

Without this patch, this raises EOF:

  (poke) var fd = open ("lalala", IOS_M_WRONLY | IOS_F_CREATE)
  (poke) int @ 0#B = 0xffffffff
  -----> at this point the IO space has 4 0xff bytes
  (poke) uint<3> @ 0#B = 2
  unhandled EOF exception

With this patch, the EOF exception is gone, but since the completing
byte cannot be read it is assumed to be 0, and therefore the resulting
data is not the right one:

  (poke) var fd = open ("lalala", IOS_M_WRONLY | IOS_F_CREATE)
  (poke) int @ 0#B = 0xffffffff
  ------> With this patch, the write above will use 0 to complete the
          first byte of the file, instead of 0xff!
  (poke) uint<3> @ 0#B = 2
  (poke) close (fd)
  (poke) .file lalala
  (poke) dump
  76543210  0011 2233 4455 6677 8899 aabb ccdd eeff  0123456789ABCDEF
  00000000: 40ff ffff                                @...

So I propose the following fix:

- If reading a completing byte is needed in a write, _and_ the IO space
  doesn't allow reading, the write operation should raise an EOF
  exception.

WDYT?

> Hi Egeyar, all.
>
> While working on other bugs we found an issue with the IOS.  We are
> proposing the following patch.
>
> Generally speaking, an IO space can be made to grow by mapping (writing)
> data past the end.
>
> Lets create an empty file:
>
>   (poke) .file/c lalala
>   (poke) dump
>   76543210  0011 2233 4455 6677 8899 aabb ccdd eeff  0123456789ABCDEF
>   00000000:  
>   (poke) iosize/#B
>   0UL
>
> Then we write past the end:
>
>   (poke) byte @ 3#B = 0xff
>   (poke) dump
>   76543210  0011 2233 4455 6677 8899 aabb ccdd eeff  0123456789ABCDEF
>   00000000: 0000 00ff                                ....
>   (poke) iosize/#B
>   4UL
>
> This is the expected behavior.
>
> However, consider what happens when you write weird integers past the
> end of the IO space, like in:
>
> (poke) uint<3> @ iosize = 2
>
> In this case, the IO subsystem has to read a byte in order to complete
> the contents of the partially written byte.  Currently this results in
> an EOF exception being raised by ios_write_u?int.  No good.
>
> In the patch attached:
>
> - We change the macro IOS_GET_C_ERR_CHCK in order to return a zero
>   completing byte instead of returning IOS_EIOFF.  This relies in the
>   fact the macro is only used in ios_write_int_common, not in
>   ios_read_int_common.
>
> - We add a new test for this.
>
> - We rename the test maps-strings-diag-4.pk to maps-strings-4.pk, and
>   adjust it accordingly.
>
> The last point is due to the fact that we are changing semantics here.
> Up to now, trying to write a value _partially_ past the end of the IO
> space resulted in an error (EIOFF, invalid offset):
>
>       |     IO space     |
>                       < value >
>
> Now, this just grows the IO space, for any kind of value: integers,
> strings, etc.
>
> Does this look good to you?
>
> commit 867d7ff1641264b560f0b0fad6ab0931cf8ab204
> Author: Jose E. Marchesi <jose.marchesi@oracle.com>
> Date:   Sat Mar 27 09:53:13 2021 +0100
>
>     ios: avoid spurious EOF when writing weird integers past the end of an IOS
>     
>     2021-03-27  Jose E. Marchesi  <jemarch@gnu.org>
>     
>             * testsuite/poke.map/ass-map-20.pk: New test.
>             * testsuite/poke.map/maps-strings-4.pk: Renamed from
>             maps-strings-diag-4.pk.
>             * testsuite/Makefile.am (EXTRA_DIST): Update accordingly.
>
> diff --git a/ChangeLog b/ChangeLog
> index 5b52061e..5453b2bf 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,10 @@
> +2021-03-27  Jose E. Marchesi  <jemarch@gnu.org>
> +
> +     * testsuite/poke.map/ass-map-20.pk: New test.
> +     * testsuite/poke.map/maps-strings-4.pk: Renamed from
> +     maps-strings-diag-4.pk.
> +     * testsuite/Makefile.am (EXTRA_DIST): Update accordingly.
> +
>  2021-03-27  Jose E. Marchesi  <jemarch@gnu.org>
>           Mohammad-Reza Nabipoor  <m.nabipoor@yahoo.com>
>  
> diff --git a/libpoke/ios.c b/libpoke/ios.c
> index 75ea05de..3e04c730 100644
> --- a/libpoke/ios.c
> +++ b/libpoke/ios.c
> @@ -32,13 +32,16 @@
>  #include "ios.h"
>  #include "ios-dev.h"
>  
> -#define IOS_GET_C_ERR_CHCK(c, io, off)                                \
> -  {                                                                \
> +#define IOS_GET_C_ERR_CHCK(c, io, off)                                 \
> +  {                                                                    \
>      uint8_t ch;                                                        \
> -    int ret = (io)->dev_if->pread ((io)->dev, &ch, 1, off);         \
> -    if (ret == IOD_EOF)                                                \
> -      return IOS_EIOFF;                                                \
> -    (c) = ch;                                                        \
> +    int ret = (io)->dev_if->pread ((io)->dev, &ch, 1, off);            \
> +    /* If the pread reports an EOF, it means the partial byte */       \
> +    /* we are writing is past the end of the IOS.  That may or */      \
> +    /* may not be supported in the underlying IOD, but in any */       \
> +    /* case that will be resolved when the completed byte is */        \
> +    /* written back.  */                                               \
> +    (c) = ret == IOD_EOF ? 0 : ch;                                     \
>    }
>  
>  #define IOS_PUT_C_ERR_CHCK(c, io, len, off)                \
> diff --git a/testsuite/Makefile.am b/testsuite/Makefile.am
> index 28968938..73b3bef4 100644
> --- a/testsuite/Makefile.am
> +++ b/testsuite/Makefile.am
> @@ -124,6 +124,7 @@ EXTRA_DIST = \
>    poke.map/ass-map-17.pk \
>    poke.map/ass-map-18.pk \
>    poke.map/ass-map-19.pk \
> +  poke.map/ass-map-20.pk \
>    poke.map/ass-map-struct-int-1.pk \
>    poke.map/func-map-1.pk \
>    poke.map/func-map-2.pk \
> @@ -276,9 +277,9 @@ EXTRA_DIST = \
>    poke.map/maps-strings-1.pk \
>    poke.map/maps-strings-2.pk \
>    poke.map/maps-strings-3.pk \
> +  poke.map/maps-strings-4.pk \
>    poke.map/maps-strings-diag-1.pk \
>    poke.map/maps-strings-diag-2.pk \
> -  poke.map/maps-strings-diag-4.pk \
>    poke.map/maps-structs-1.pk \
>    poke.map/maps-structs-2.pk \
>    poke.map/maps-structs-3.pk \
> diff --git a/testsuite/poke.map/ass-map-20.pk 
> b/testsuite/poke.map/ass-map-20.pk
> new file mode 100644
> index 00000000..5fb97e92
> --- /dev/null
> +++ b/testsuite/poke.map/ass-map-20.pk
> @@ -0,0 +1,7 @@
> +/* { dg-do run } */
> +
> +/* { dg-command {.mem foo} } */
> +/* { dg-command {var is = iosize} } */
> +/* { dg-command {uint<3> @ is = 2} } */
> +/* { dg-command {uint<3> @ is} } */
> +/* { dg-output ".uint<3>. 2" } */
> diff --git a/testsuite/poke.map/maps-strings-4.pk 
> b/testsuite/poke.map/maps-strings-4.pk
> new file mode 100644
> index 00000000..3ced1d4c
> --- /dev/null
> +++ b/testsuite/poke.map/maps-strings-4.pk
> @@ -0,0 +1,11 @@
> +/* { dg-do run } */
> +/* { dg-data {c*} {0x66 0x6f 0x6f 0x00  0x50 0x60 0x70 0x80   0x90 0xa0 0xb0 
> 0xc0} } */
> +
> +/* Writing a string partially past the end of the IO space is allowed.
> +   The IO space grows.  */
> +
> +/* { dg-command { string @ 8#B + 1#b = "quux" } } */
> +/* { dg-command { iosize/#B } } */
> +/* { dg-output "13UL" } */
> +/* { dg-command { string @ 8#B + 1#b } } */
> +/* { dg-output "\n\"quux\"" } */
> diff --git a/testsuite/poke.map/maps-strings-diag-4.pk 
> b/testsuite/poke.map/maps-strings-diag-4.pk
> deleted file mode 100644
> index 5d7943d6..00000000
> --- a/testsuite/poke.map/maps-strings-diag-4.pk
> +++ /dev/null
> @@ -1,5 +0,0 @@
> -/* { dg-do run } */
> -/* { dg-data {c*} {0x66 0x6f 0x6f 0x00  0x50 0x60 0x70 0x80   0x90 0xa0 0xb0 
> 0xc0} } */
> -
> -/* { dg-command { try string @ (8*8+1)#b = "quux"; catch if E_eof { printf 
> "caught\n"; } } } */
> -/* { dg-output "caught" } */