#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: review Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by sbrandt):

* status: new => review

#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by rhaas):

* status: reviewed_ok => reopened

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.

#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------

Comment (by rhaas):

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

#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------

Comment (by rhaas):

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)

#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------

Comment (by rhaas):

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.

#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------

Comment (by rhaas):

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. 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.

#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------

Comment (by rhaas):

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.

#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------

Comment (by rhaas):

Replying to [comment:14 sbrandt]:

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

parses. I see. Thank you.

#2106: Make ParseFile.c more robust -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------+----------------------------------------------------

Comment (by sbrandt):

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

#2106: Make ParseFile.c more robust -----------------------------+--------------------------------- Reporter: Erik Schnetter | Owner: (none) Type: defect | Status: reopened Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------------+---------------------------------

Comment (by Erik Schnetter):

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.

#2106: Make ParseFile.c more robust -----------------------------+--------------------------------- Reporter: Erik Schnetter | Owner: (none) Type: defect | Status: review Priority: unset | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: -----------------------------+--------------------------------- Changes (by Roland Haas):

* status: reopened => review