Make ParseFile.c more robust

Issue #2106 closed
Erik Schnetter created an issue

Comments (23)

  1. Roland Haas
    • changed status to open
    • removed comment

    The error check is incorrect. fseek returns "-1" on error not "a negative number". This really only matters once we encounter a parfile that is >2GB in size, but still.

  2. Steven R. Brandt
    • removed comment

    Roland, I'm confused. Fseek only returns 0 or -1. What difference should it make whether the file is bigger than 2GB?

  3. Roland Haas
    • removed comment

    Right, sorry. I was thinking of ftell. So the 2GB does not apply. Would still be nice to check for the documented return code.

  4. Roland Haas
    • removed comment

    Yes, please. The "< 0" vs. "== -1" is the only thing missing. In line 347

    if (*filesize < 0)
    

    it it probably best to actually write

    if (*filesize == (unsigned long int)-1)
    

    since in the current version an aggressively optimizing compiler might do the following steps: * filesize is declared to be unsigned long *filesize * unsigned values can never be negative * the if statement can never trigger * compiler removes the error check (though usually at least generating a warning)

  5. Roland Haas
    • removed comment

    Uff, I see. "usual arithmetic conversion" is really murky to me. So I try to err on the side of not relying on it.

    I think I would agree with this assessment if the conversion happened in in the if statement. However the actual code is:

    static char *ReadFile(FILE *file, unsigned long *filesize)
    {
    ...
    *filesize = ftell(file);
    if (*filesize < 0)
    {
      // do stuff
    

    so the result of ftell (a long int) is converted to unsigned long int when it is assigned to *filesize. Since filesize is known to be "unsigned long int" the optimization seems valid to me (namely an unsigned int comparison with <0 will always fail).

    The cast of -1 to unsigned long int that I added is just to avoid the warning about comparing signed and unsigned values.

    A test code:

    void foo()
    {
      unsigned int bar = 1;
      if(bar < 0) {
        baz();
      }
    
      if(bar == -1) {
        baz();
      }
    }
    

    gives me two warnings on gcc 7.3.0 when using -Wextra (but not eg -Wall):

    rhaas@ekohaes8:.../rhaas/tmp$ gcc -Wall -Wextra -c  compare.c
    compare.c: In function ‘foo’:
    compare.c:4:10: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
       if(bar < 0) {
              ^
    compare.c:8:10: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       if(bar == -1) {
              ^~
    

    Note the first warning about the comparison always being false.

  6. Steven R. Brandt
    • removed comment

    Why don't we just add a variable?

    auto fresult = ftell(...)
    if(fresult < 0) {
      *filesize = 0;
      fprint(fstder,...)
      ...
    } else {
      *filesize = fresult;
    }
    
  7. Roland Haas
    • removed comment

    Not sure how good that actually is

    1. *filesize does not need to be set to anything in case of failures since we call CCTK_Error anyway. (Edited): Not quite correct, or at least only in the same correct that several levels up in the call tree do we call CCTK_VWarn(0, ...). Not that it is a bad idea to set to an "invalid" value in that case, though maybe "-1" and making filesize a long int * would be better since the file could actually be empty.
    2. this it still seems to compare the result of ftell to <0 instead of the documented value of -1. While I strongly doubt that in this particular way of coding this comparison, there will be a problem (since it would require that the standard uses another negative value, other than -1, to indicate a non-error return of ftell) I do not quite see why we would deliberately test for a condition that is (at least in principle even if not in practise) different from the one documented.
  8. Steven R. Brandt
    • removed comment

    OK, how about we change ReadFile(FILE *file,signed long *filessize) to ReadFile(FILE *file,signed long *filesize) and the <0 to == -1. Can we then move to review_ok?

  9. Roland Haas
    • removed comment

    Probably yes. I just hope that changing to signed long does not create a host of new warnings about mixing singed and unsigned types in the remainder of the code. Sorry for being so picky about all of this, in particular since this is all a very minor part of the code (is it even still used by the piraha parser code?) and the discussion by now has taken much more time than writing the patch likely did. The original patch though I think did have an actual bug in the comparison of *filesize to <0, so the review, mostly anyway, served its intended purpose of catching bugs.

    Would it be possible to update the pull request with the actual proposed change, please? Otherwise it seems to me that there is some danger of not quite agreeing on the same thing to commit.

  10. Steven R. Brandt
    • removed comment

    Yes, this source still gets called. It reads the buffer that Piraha parses.

  11. Roland Haas
    • removed comment

    Replying to [comment:14 sbrandt]:

    Yes, this source still gets called. It reads the buffer that Piraha parses. I see. Thank you.

  12. Steven R. Brandt
    • removed comment

    Erik, this branch is owned by you rather than cactuscode. Can you change and commit?

  13. Erik Schnetter reporter
    • removed comment

    I've changed the variable from unsigned long to long.

    I have not changed the error tests to check against -1, since all negative values indicate errors, in the sense that our malloc(filesize) later on will fail for any negative file size, independent of whether the file size is -1 indicating an error in ftell, or whether ftell returns another negative number for some other reason.

  14. Log in to comment