JayFoxRox commented 5 years ago

This was found on pdclib for nxdk (homebrew toolchain for the original Xbox). The nxdk pdclib is very close to the example reference implementation, so I believe the following is a flaw in the pdclib design, not in our implementation.

Here's code which triggers the bug:

(I'll refer to the machines kernel / filesystem driver as "platform backend")

#include <stdio.h>

#ifdef NXDK
// Hack for original Xbox: Use a framebuffer draw instead of stdout
#include <xboxrt/debug.h>
#define printf(fmt, ...) debugPrint(fmt, __VA_ARGS__)
#endif

int main() {

  FILE* f = fopen("default.xbe" ,"rb");
  // platform backend at offset 0
  // pdclib at offset 0

  char buf[10];
  fread(buf, 1, 10, f);
  // platform backend was forced to fill the entire buffer, so platform backend is at 1024 (_PDCLIB_BUFSIZ)
  // pdclib has 1024 buffer, and knows platform backend is at 1024, but, correctly, pdclib only moves to offset 10 in that buffer

  size_t a = ftell(f);
  printf("A: %zu\n", a);
  // Still returns 10 (handled internally by pdclib; recovered by doing a bit of math with known values)

  // !!! The state is still good

  fseek(f, 0, SEEK_CUR);
  // platform backend reports new offset as 1024
  // pdclib trusts this and is now also at offset 1024 (should be at 10)

  // !!! The state is now broken

  size_t b = ftell(f);
  printf("B: %zu\n", b);
  // Suddenly returns 1024

  while(1);

  return 0;
}

Explanation:

When reading, this remembers how much of the buffer it read so pdclib knows the real offset in the file (so even if pdclib buffers 1024 bytes, it knows it is still at offset 0; and if it reads X bytes of the buffer, it's at offset X). Meanwhile, the underlying platform backend (kernel / filesystem driver) will have seeked.
When doing a relative seek, the platform backend only knows about its own information. It will have it's own cursor at the end of where pdclib stopped filling the buffer from. That will be returned from here (Something like _PDCLIB_BUFSIZ = 1024) and used for relative seeks. As far as the platform backend is concerned this is the current cursor position.
When the platform code returns, pdclib will forget about its own buffer offset and accept the platform backend cursor as true fact. pdclib has now accidentally moved from knowing where it is in the buffer, to another location behind the buffer.

I hope this will be reviewed / addressed soon.

I did not check wether this also affects other functions, like unget, write, ..; I also did not think about other cases like SEEK_SET or SEEK_END yet.

DevSolar commented 5 years ago

Hello Jannik,thank you for pointing this out. I am "on the road" right now but will have a look at the issue ASAP.As long as the fix will be somewhat simple, I think I will have it done within this week (possibly together with the freopen() issue that has yet to be addressed). If it turns out I have to redesign the whole buffering solution, it might take a bit longer.While PDCLib is still pre-alpha, I take issues that are directly affecting users of my work very seriously, and will consider this a priority item. Thanks again for the detailed description, I am sure this will help immensely nailing it down.Kind regards,--Martin Bautesolar@rootdirectory.deVon meinem Samsung Galaxy Smartphone gesendet. -------- Ursprüngliche Nachricht --------Von: Jannik Vogel notifications@github.com Datum: 19.05.19 12:14 (GMT+01:00) An: DevSolar/pdclib pdclib@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Betreff: [DevSolar/pdclib] Design flaw / logic error in _PDCLIB_seek (#3) This was found on pdclib for nxdk (homebrew toolchain for the original Xbox). The nxdk pdclib is very close to the example reference implementation, so I believe the following is a flaw in the pdclib design, not in our implementation. Here's code which triggers the bug: (I'll refer to the machines kernel / filesystem driver as "platform backend")

include

ifdef NXDK

// Hack for original Xbox: Use a framebuffer draw instead of stdout

include <xboxrt/debug.h>

define printf(fmt, ...) debugPrint(fmt, __VA_ARGS__)

endif

int main() {

FILE* f = fopen("default.xbe" ,"rb"); // platform backend at offset 0 // pdclib at offset 0

char buf[10]; fread(buf, 1, 10, f); // platform backend was forced to fill the entire buffer, so platform backend is at 1024 (_PDCLIB_BUFSIZ) // pdclib has 1024 buffer, and knows platform backend is at 1024, but, correctly, pdclib only moves to offset 10 in that buffer

size_t a = ftell(f); printf("A: %zu\n", a); // Still returns 10 (handled internally by pdclib; recovered by doing a bit of math with known values)

// !!! The state is still good

fseek(f, 0, SEEK_CUR); // platform backend reports new offset as 1024 // pdclib trusts this and is now also at offset 1024 (should be at 10)

// !!! The state is now broken

size_t b = ftell(f); printf("B: %zu\n", b); // Suddenly returns 1024

while(1);

return 0; } Explanation:

When reading, this remembers how much of the buffer it read so pdclib knows the real offset in the file (so even if pdclib buffers 1024 bytes, it knows it is still at offset 0; and if it reads X bytes of the buffer, it's at offset X). Meanwhile, the underlying platform backend (kernel / filesystem driver) will have seeked.

When doing a relative seek, the platform backend only knows about its own information. It will have it's own cursor at the end of where pdclib stopped filling the buffer from. That will be returned from here (Something like _PDCLIB_BUFSIZ = 1024) and used for relative seeks. As far as the platform backend is concerned this is the current cursor position.

When the platform code returns, pdclib will forget about its own buffer offset and accept the platform backend cursor as true fact. pdclib has now accidentally moved from knowing where it is in the buffer, to another location behind the buffer.

I hope this will be reviewed / addressed soon. I did not check wether this also affects other functions, like unget, write, ..; I also did not think about other cases like SEEK_SET or SEEK_END yet.

—You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or mute the thread. [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/DevSolar/pdclib/issues/3?email_source=notifications\u0026email_token=ACVZVQ6EVEAYKZKKU7L3JXTPWESBTA5CNFSM4HN4EEYKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GUSNDUA", "url": "https://github.com/DevSolar/pdclib/issues/3?email_source=notifications\u0026email_token=ACVZVQ6EVEAYKZKKU7L3JXTPWESBTA5CNFSM4HN4EEYKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GUSNDUA", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

DevSolar commented 5 years ago

Hey Jannik,

could you please verify that this patch fixes the issue?

diff --git a/functions/stdio/fseek.c b/functions/stdio/fseek.c index 9ad1f53..a3a5a8f 100644 --- a/functions/stdio/fseek.c +++ b/functions/stdio/fseek.c @@ -35,6 +35,11 @@ int fseek( struct _PDCLIB_file_t * stream, long offset, int whence ) stream->status &= ~ ( _PDCLIB_FREAD | _PDCLIB_FWRITE ); }

if ( whence == SEEK_CUR )
{
offset -= ( ( (int)stream->bufend - (int)stream->bufidx ) + stream->ungetidx );
}
rc = ( _PDCLIB_seek( stream, offset, whence ) != EOF ) ? 0 : EOF; _PDCLIB_UNLOCK( stream->mtx ); return rc; diff --git a/functions/stdio/ftell.c b/functions/stdio/ftell.c index dbac1cf..302fa82 100644 --- a/functions/stdio/ftell.c +++ b/functions/stdio/ftell.c @@ -102,6 +102,9 @@ int main( void ) TESTCASE_NOREG( fh->bufidx == 0 ); / Reading back first character after rewind for basic read check / TESTCASE( fgetc( fh ) == '1' );
/ Check that SEEK_CUR actually uses internal position /
TESTCASE( fseek( fh, 1l, SEEK_CUR ) == 0 );
TESTCASE( ftell( fh ) == 2l ); / TODO: t.b.c. / TESTCASE( fclose( fh ) == 0 ); return TEST_RESULTS;

Thank you again for bringing this issue to my attention. I have been sloppy with the positioning tests, I realized. Will keep that in mind and on my to-do list.

Kind regards,

Martin Baute solar@rootdirectory.de

Am 19.05.2019 12:14, schrieb Jannik Vogel:

This was found on pdclib for nxdk (homebrew toolchain for the original Xbox). The nxdk pdclib [1] is very close to the example reference implementation, so I believe the following is a flaw in the pdclib design, not in our implementation.

Here's code which triggers the bug:

(I'll refer to the machines kernel / filesystem driver as "platform backend")

include

ifdef NXDK

// Hack for original Xbox: Use a framebuffer draw instead of stdout

include

define printf(fmt, ...) debugPrint(fmt, __VA_ARGS__)

endif

int main() {

FILE* f = fopen("default.xbe" ,"rb"); // platform backend at offset 0 // pdclib at offset 0

char buf[10]; fread(buf, 1, 10, f); // platform backend was forced to fill the entire buffer, so platform backend is at 1024 (_PDCLIB_BUFSIZ) // pdclib has 1024 buffer, and knows platform backend is at 1024, but, correctly, pdclib only moves to offset 10 in that buffer

size_t a = ftell(f); printf("A: %zun", a); // Still returns 10 (handled internally by pdclib; recovered by doing a bit of math with known values)

// !!! The state is still good

fseek(f, 0, SEEK_CUR); // platform backend reports new offset as 1024 // pdclib trusts this and is now also at offset 1024 (should be at 10)

// !!! The state is now broken

size_t b = ftell(f); printf("B: %zun", b); // Suddenly returns 1024

while(1);

return 0; }

EXPLANATION:

*

When reading, this remembers how much of the buffer it read [2] so pdclib knows the real offset in the file (so even if pdclib buffers 1024 bytes, it knows it is still at offset 0; and if it reads X bytes of the buffer, it's at offset X). Meanwhile, the underlying platform backend (kernel / filesystem driver) will have seeked. *

When doing a relative seek, the platform backend only knows about its own information. It will have it's own cursor at the end of where pdclib stopped filling the buffer from. That will be returned from here [3] (Something like _PDCLIB_BUFSIZ = 1024) and used for relative seeks. As far as the platform backend is concerned this is the current cursor position. *

When the platform code returns, pdclib will forget about its own buffer offset and accept the platform backend cursor as true fact [4]. pdclib has now accidentally moved from knowing where it is in the buffer, to another location behind the buffer.

I hope this will be reviewed / addressed soon.

I did not check wether this also affects other functions, like unget, write, ..; I also did not think about other cases like SEEK_SET or SEEK_END yet.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub [5], or mute the thread [6].

Links:

[1] https://github.com/XboxDev/nxdk-pdclib/tree/nxdk/platform/xbox [2]

https://github.com/XboxDev/nxdk-pdclib/blob/1040a66ded634793a29a2ee80385d524c29ab05f/platform/xbox/functions/_PDCLIB/_PDCLIB_fillbuffer.c#L38-L40 [3]

https://github.com/XboxDev/nxdk/blob/82e4860c2f5a358677f60000e40f8f00529e5e8e/lib/hal/fileio.c#L429 [4]

https://github.com/XboxDev/nxdk-pdclib/blob/1040a66ded634793a29a2ee80385d524c29ab05f/platform/xbox/functions/_PDCLIB/_PDCLIB_seek.c#L36-L39 [5]

https://github.com/DevSolar/pdclib/issues/3?email_source=notifications|+|amp|+|email_token=ACVZVQ6EVEAYKZKKU7L3JXTPWESBTA5CNFSM4HN4EEYKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GUSNDUA [6]

https://github.com/notifications/unsubscribe-auth/ACVZVQYUEYHAJR43CI3KZGTPWESBTANCNFSM4HN4EEYA

JayFoxRox commented 5 years ago

I had someone else test it for me, and it seems to work fine. Thanks :+1:

DevSolar / pdclib

Design flaw / logic error in _PDCLIB_seek #3

include

ifdef NXDK

include <xboxrt/debug.h>

define printf(fmt, ...) debugPrint(fmt, __VA_ARGS__)

endif

Kind regards,

include

ifdef NXDK

include

define printf(fmt, ...) debugPrint(fmt, __VA_ARGS__)

endif

Links: