Skip to content
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.

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

It's UB to have a call to va_start without a corresponding call to va_end #51

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

camel-cdr
Copy link

@camel-cdr camel-cdr commented May 1, 2021

This is just a tiny nitpick, but Annex J says:

The behavior is undefined in the following circumstances: [...] The va_start or va_copy macro is invoked without a corresponding invocation of the va_end macro in the same function, or vice versa

So its technically UB, even if you call exit(). This will probably not cause any problems, but it might be worth having the code be more compliant.

(Also thanks for the amazing project!)

@camel-cdr
Copy link
Author

camel-cdr commented May 1, 2021

I just realized that fixing this will be a bit more involved, as early versions of verror_at call exit internally, and I'm not sure if is UB:

static void verror_at(char *loc, char *fmt, va_list ap) {
  int pos = loc - current_input;
  fprintf(stderr, "%s\n", current_input);
  fprintf(stderr, "%*s", pos, ""); // print pos spaces.
  fprintf(stderr, "^ ");
  vfprintf(stderr, fmt, ap);
  fprintf(stderr, "\n");
  exit(1);
}

static void error_at(char *loc, char *fmt, ...) {
  va_list ap;
  va_start(ap, fmt);
  verror_at(loc, fmt, ap);
  va_end(ap, fmt); // <- new addition
}

I think that invocation indicates, that the va_end needs to be executed, hence the above would e UB, and it might be a good idea to update the older commits to not exit in verror_at.

(The effort this would take might not be worth it though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant