fread and ungetc bug fix

[SSpeare]

While working on lua support we discovered what appears to be a bug in stdio.c. The data that is putback on the stream by ungetc() is not honored by some other functions. Notably, fread() will ignore the putback which breaks lua file loading. We're proposing the below fix. Any input would be appreciated:

Code: Select all

size_t fread(void *buf, size_t r, size_t n, FILE *stream)
{
  size_t ret;

  switch(stream->type) {
    case STD_IOBUF_TYPE_NONE:
    case STD_IOBUF_TYPE_GS:
    case STD_IOBUF_TYPE_SIO:
    case STD_IOBUF_TYPE_STDOUTHOST:
      /* cannot read from stdout or stderr. */
      ret = 0;
      break;
    default:
      /* attempt to read from the stream file. */
      if (stream->has_putback) {
        buf[0] = stream->putback;
        buf++;
        stream->has_putback = 0;
        /* subtract 1 from r * n to avoid buffer overflow */
        ret = (_ps2sdk_read(stream->fd, buf, (int)((r * n) -1 ))) / (int)r);
      } else {
        ret = (_ps2sdk_read(stream->fd, buf, (int)(r * n)) / (int)r);
      }
  }
  return (ret);
} 

Something else I noticed:
fread() will return partial records if your record size is >1. That seems odd to me, but that is the current behavior so I guess we should just leave it alone for the moment.

e.g.
If I ask to read 10 records of size 4, and fread() finds 38 bytes, I would expect it to return 9 records of size 4 (36 bytes), not 38 bytes which is 9 records plus a half record. The return value of fread() will be 9 because it is forced to an int, so it will lie about how much data it read. Then if you fread() again you will have mis-aligned data because you have already read 1/2 of the record, but you didn't know that because the function lied.

I'm not sure what the behavior of fread() is in a standard implementation, though... I haven't used C extensively in a long time. This may be just a caveat.

It should still probably subtract the 1 from (r * n)... Either way it won't fix the above problem.

[evilo]

Furthermore, as also said previously, the stream->as_putback flag is not checked (where it should be) in fwrite, ftell, fgetpos and not reseted in some others (like fsetpos, etc..)

[evilo]

so this is the final working correction

Code: Select all

size_t fread(void *buf, size_t r, size_t n, FILE *stream)
{
  size_t ret;
  
  switch(stream->type) {
    case STD_IOBUF_TYPE_NONE:
    case STD_IOBUF_TYPE_GS:
    case STD_IOBUF_TYPE_SIO:
    case STD_IOBUF_TYPE_STDOUTHOST:
      /* cannot read from stdout or stderr. */
      ret = 0;
      break;
    default:
      /* attempt to read from the stream file. */
      if (stream->has_putback) {
      	unsigned char *ptr = (unsigned char *)buf;
        *ptr = stream->putback;
        buf = ptr + 1;
        stream->has_putback = 0;
        /* subtract 1 from r * n to avoid buffer overflow */
        ret = (_ps2sdk_read(stream->fd, buf, (int)((r * n) -1 )) / (int)r);
      } else {
        ret = (_ps2sdk_read(stream->fd, buf, (int)(r * n)) / (int)r);
      }
  }
  return (ret);
}

however, when running the regression tests, fgets seems to be broken, I dunno if it's linked since i didn't ran it before my modification. i'll put the update in svn once fixed.

cheers.

[EDIT] forgot to mention that the blank line at the start of a lua script is not required anymore :)

[evilo]

fgets function seems to be fixed even if the following test (test #2 from the regression tests) always fail..

Code: Select all

ret = fgets(buf, sizeof(buf), fp);
fclose(fp);

if (ret!=buf || strcmp(buf, "hello world\n") != 0)
{
	return "read wrong data from file";
}

but if buf and ret are printed, they are both equals...

anyway I still have an issue as well with ftell/fseek test (test #5 of the regression test) but since I'm too tired, i'm stoping for today ...

here is the current changes I made to libc, if you want to have a look (in case you see how to correct it (run the regression test to see it) :
http://psxdev.info/evilo/download/libc/src/stdio.c

evilo

evilo.

[Orfax]

While working on lua support we discovered what appears to be a bug in stdio.c.

Yes I think there are a few more still (I will have a look at those when I get around to them).

While it may have nothing to do with what you guys have found, I just wanted to let you know I recently did some changes to the libc (and stdio.c). I want to make sure the changes I have done, haven't caused some side effects (I don't think they have, these are new issues).
I *have* modified fgets().

I'm not sure what the behavior of fread()

In the man page (on cygwin) it just says fread returns the number of elements it successfully reads. You may want to set up a test case showing the behaviour and try it on a number of different fread implementations to see what is returned (I tend to use ps2dev, cygwin and AIX because they are what I have access to).

but if buf and ret are printed, they are both equals...

fgets returns either the buffer passed in (if values were read) or NULL (if it was not able to read anything). fgets() was not putting the newline in the buffer when it was read. This is what I fixed with it. I did not change the regression tests (and I really should have). Is this why the regression test is failing?

Furthermore, as also said previously, the stream->as_putback flag is not checked

Also please check the scanf type functions. They also put a character back onto the stream. I had the issue where they were putting the wrong character back.

For more information about what I did please see the following topic:
http://forums.ps2dev.org/viewtopic.php?t=4158

[evilo]

Changes are commited into svn . I checked scanf and it seems ok for me, though I didn't tested to be honest.

evilo