Skip to content

Various fixes related to makePromise() pointing behind EOF #4266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Various fixes related to makePromise() pointing behind EOF #4266

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9157b7f
main: perform parsing with a subparser only when pushArea() succeeds
techee Jun 2, 2025
218edc4
mio: Check validity of parameters passed to mio_new_mio()
techee Jun 2, 2025
1816905
mio: update position when using mio_seek() even when pointing behind EOF
techee Jun 2, 2025
2cfdfd6
Revert "mio: update position when using mio_seek() even when pointing…
techee Jun 3, 2025
f9969f5
main: add more checks to pushArea()
techee Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions main/mio.c
View file
Open in desktop
Copy link
Member

@b4n b4n Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this change is not a good idea. I see a few issues, which basically stem from the C standard.

First, the C standard does not specify this behavior. Quoting C17 N2176:

Description

The fseek function sets the file position indicator for the stream pointed to by stream. If a read or
write error occurs, the error indicator for the stream is set and fseek fails.

For a binary stream, the new position, measured in characters from the beginning of the file, is
obtained by adding offset to the position specified by whence. The specified position is the
beginning of the file if whence is SEEK_SET, the current value of the file position indicator if
SEEK_CUR, or end-of-file if SEEK_END. A binary stream need not meaningfully support fseek calls
with a whence value of SEEK_END.

For a text stream, either offset shall be zero, or offset shall be a value returned by an earlier
successful call to the ftell function on a stream associated with the same file and whence shall be
SEEK_SET.

After determining the new position, a successful call to the fseek function undoes any effects of the
ungetc function on the stream, clears the end-of-file indicator for the stream, and then establishes
the new position. After a successful fseek call, the next operation on an update stream may be
either input or output.

With After determining the new position, a successful call to the fseek function […] then establishes
the new position, one could even read that it should not change the position if the call is not successful (and can it be if it's not actually honored?).

But at any rate, as there is nothing specifying this behavior, it can't be relied upon for FILE-based streams, and thus I don't see any value of changing the behavior for memory streams, it just adds a more or less random behavior for a specific case. If it's not standard, the caller shouldn't rely on it, or it's gonna be brittle.

Additionally, it's not something my libc does. I doesn't behave like you suggest (here I have glibc 2.36):
Using the very crude

#include <stdlib.h>
#include <stdio.h>

int main(int argc, char **argv)
{
  FILE *fp = fopen(argv[1], "rb");
  
  if (! fp)
    return 1;

  long off = atol(argv[2]);

  fprintf(stderr, "initial position: %ld\n", ftell(fp));
  fprintf(stderr, "seeking to %ld: %d\n", off, fseek(fp, off, SEEK_SET));
  fprintf(stderr, "ftell(): %ld\n", ftell(fp));
  fprintf(stderr, "seeking to +32: %d\n", fseek(fp, 32, SEEK_CUR));
  fprintf(stderr, "ftell(): %ld\n", ftell(fp));
  
  fprintf(stderr, "seeking to end: %d\n", fseek(fp, 0, SEEK_END));
  fprintf(stderr, "ftell(): %ld\n", ftell(fp));

  fclose(fp);
  
  return 0;
}

and running it:

$ ./seek.out seek.c 1000
initial position: 0
seeking to 1000: 0
ftell(): 1000
seeking to +32: 0
ftell(): 1032
seeking to end: 0
ftell(): 613

You can clearly see that it sought (hehe, seek()ed) past the end: when seeking to SEEK_END, I get 613, but if I seek to 1000 I get that 1000 (which is then past the end), and I can even further SEEK_CUR happily.

So in the end I'd rather not have this change any internal on failure, and leave it to the caller. Originally MIO was designed to mimic as closely as possible the equivalent C library calls, not adding any fanciness anywhere it, and I don't think it should deviate to what can be expected from the C library.

At some point it had tests to compare against the C library itself, maybe we should re-introduce this in uctags -- not that it would catch anything here, as the test would have not to depend on unspecified behavior, but we could verify it's not diverging on specified behavior.

Copy link
Contributor Author

@techee techee Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it was just an idea inspired by the fact that ctags uses mio_seek() and mio_setpos() without any checks of the returned value and that I thought that in the error cases it would be better to be "closer" to the provided value. The right thing of course is to properly check the return values and recover accordingly.

Let's discard this patch.

Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,9 @@
MIO *submio;
size_t r;

if (start < 0 || size < -1)
return NULL;

original_pos = mio_tell (base);

if (size == -1)
Expand Down Expand Up @@ -1079,8 +1082,13 @@
switch (whence)
{
case SEEK_SET:
if (offset < 0 || (size_t)offset > mio->impl.mem.size)
if (offset < 0)
errno = EINVAL;

Check warning on line 1086 in main/mio.c

View check run for this annotation

Codecov / codecov/patch

main/mio.c#L1086 

Added line #L1086 was not covered by tests
else if ((size_t)offset > mio->impl.mem.size)
{
errno = EINVAL;
mio->impl.mem.pos = mio->impl.mem.size;

Check warning on line 1090 in main/mio.c

View check run for this annotation

Codecov / codecov/patch

main/mio.c#L1090 

Added line #L1090 was not covered by tests
}
else
{
mio->impl.mem.pos = (size_t)offset;
Expand All @@ -1089,10 +1097,12 @@
break;

case SEEK_CUR:
if ((offset < 0 && (size_t)-offset > mio->impl.mem.pos) ||
mio->impl.mem.pos + (size_t)offset > mio->impl.mem.size)
if (offset < 0 && (size_t)-offset > mio->impl.mem.pos)
errno = EINVAL;

Check warning on line 1101 in main/mio.c

View check run for this annotation

Codecov / codecov/patch

main/mio.c#L1101 

Added line #L1101 was not covered by tests
else if (mio->impl.mem.pos + (size_t)offset > mio->impl.mem.size)
{
errno = EINVAL;
mio->impl.mem.pos = mio->impl.mem.size;

Check warning on line 1105 in main/mio.c

View check run for this annotation

Codecov / codecov/patch

main/mio.c#L1105 

Added line #L1105 was not covered by tests
}
else
{
Expand Down Expand Up @@ -1279,7 +1289,10 @@
rv = -1;

if (pos->impl.mem > mio->impl.mem.size)
{
errno = EINVAL;
mio->impl.mem.pos = mio->impl.mem.size;

Check warning on line 1294 in main/mio.c

View check run for this annotation

Codecov / codecov/patch

main/mio.c#L1294 

Added line #L1294 was not covered by tests
}
else
{
mio->impl.mem.ungetch = EOF;
Expand Down
12 changes: 7 additions & 5 deletions main/parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4322,7 +4322,7 @@ extern bool runParserInArea (const langType language,
unsigned long sourceLineOffset,
int promise)
{
bool tagFileResized;
bool tagFileResized = false;

verbose ("runParserInArea: %s; "
"file: %s, "
Expand All @@ -4334,13 +4334,15 @@ extern bool runParserInArea (const langType language,
startLine, startCharOffset, sourceLineOffset,
endLine, endCharOffset);

pushArea (doesParserRequireMemoryStream (language),
if (pushArea (doesParserRequireMemoryStream (language),
startLine, startCharOffset,
endLine, endCharOffset,
sourceLineOffset,
promise);
tagFileResized = createTagsWithFallback1 (language, NULL);
popArea ();
promise))
{
tagFileResized = createTagsWithFallback1 (language, NULL);
popArea ();
}
return tagFileResized;

}
Expand Down
9 changes: 7 additions & 2 deletions main/read.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,7 @@ extern char *readLineFromBypass (
return data.result;
}

extern void pushArea (
extern bool pushArea (
bool useMemoryStreamInput,
unsigned long startLine, long startColumn,
unsigned long endLine, long endColumn,
Expand All @@ -1415,7 +1415,7 @@ extern void pushArea (
{
File.thinDepth++;
verbose ("push thin area (%d)\n", File.thinDepth);
return;
return true;
}
error(WARNING, "INTERNAL ERROR: though pushing MEMORY based thin area, "
"underlying area is a FILE base: %s@%s",
Expand Down Expand Up @@ -1448,6 +1448,9 @@ extern void pushArea (

mio_setpos (File.mio, &original);

if (q <= p)
return false;

invalidatePatternCache();

size_t size = q - p;
Expand All @@ -1473,6 +1476,8 @@ extern void pushArea (

File.input.lineNumberOrigin = ((startLine == 0)? 0: startLine - 1);
File.source.lineNumberOrigin = ((sourceLineOffset == 0)? 0: sourceLineOffset - 1);

return true;
}

extern bool isAreaStacked (void)
Expand Down
2 changes: 1 addition & 1 deletion main/read_p.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ extern char *readLineFromBypass (vString *const vLine, MIOPos pos, long *const o
* args (endColumn): [absolute]
* args (sourceLineOffset): [buggy]
*/
extern void pushArea (
extern bool pushArea (
bool useMemoryStreamInput,
unsigned long startLine, long startColumn,
unsigned long endLine, long endColumn,
Expand Down
Loading