Modify

Opened 10 months ago

Last modified 8 months ago

#2106 reopened defect

Make ParseFile.c more robust

Reported by: Erik Schnetter Owned by:
Priority: unset Milestone:
Component: Cactus Version: development version
Keywords: Cc:

Attachments (0)

Change History (17)

comment:1 Changed 9 months ago by Steven R. Brandt

Status: newreview

comment:2 Changed 9 months ago by Steven R. Brandt

Status: reviewreviewed_ok

comment:3 Changed 9 months ago by Roland Haas

Status: reviewed_okreopened

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.

comment:4 Changed 8 months ago by Steven R. Brandt

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

comment:5 Changed 8 months ago by Roland Haas

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

comment:6 Changed 8 months ago by Steven R. Brandt

All right, if we change "< 0" to "== -1" can we move this to review_ok?

comment:7 Changed 8 months ago by Roland Haas

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)

comment:8 Changed 8 months ago by Erik Schnetter

That's not a valid optimization. The "usual arithmetic conversions" mean that both LHS and RHS of the comparison operator are converted to unsigned before the comparison <https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules>.

comment:9 Changed 8 months ago by Roland Haas

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.

comment:10 Changed 8 months ago by Steven R. Brandt

Why don't we just add a variable?

auto fresult = ftell(...)
if(fresult < 0) {
  *filesize = 0;
  fprint(fstder,...)
  ...
} else {
  *filesize = fresult;
}

comment:11 Changed 8 months ago by Roland Haas

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.
Last edited 8 months ago by Roland Haas (previous) (diff)

comment:12 Changed 8 months ago by Steven R. Brandt

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?

comment:13 Changed 8 months ago by Roland Haas

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.

comment:14 Changed 8 months ago by Steven R. Brandt

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

comment:15 in reply to:  14 Changed 8 months ago by Roland Haas

Replying to sbrandt:

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

I see. Thank you.

comment:16 Changed 8 months ago by Erik Schnetter

ftell returns a long. Please update the code so that ReadFile also returns a long. Then the comparison with 0 will work fine.

comment:17 Changed 8 months ago by Steven R. Brandt

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The ticket will remain with no owner.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from (none) to the specified user.
The owner will be changed from (none) to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.