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

handle null-byte case in scanformat (fixes #1105) #1106

Merged
merged 2 commits into from
Apr 22, 2023

Conversation

CosmicToast
Copy link
Contributor

When there is no format to be found after a %, get_fmt_mapping returns NULL.
It then gets called against strlen, which is a typical SEGV.
Check for NULL aginst mapping, which signals a null format being specified.

Note that there are other edge-conditions present.
Namely, consider: (printf "hello % " "dave") (format is still null).
Meanwhile, (printf "hello % s" "dave") (outputs "hello dave").

When there is no format to be found after a %, get_fmt_mapping returns
NULL. It then gets called against strlen, which is a typical SEGV.
Check for NULL aginst mapping, which signals a null format being
specified.
Copy link
Member

@pepe pepe left a comment

Choose a reason for hiding this comment

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

I would love to have a bit of information about what happened. And maybe that is just me.

@pepe
Copy link
Member

pepe commented Apr 20, 2023

I mean in the error message.

@sogaiu
Copy link
Contributor

sogaiu commented Apr 20, 2023

Ah, I finally get it, @pepe, you mean that may be "invalid format (found null)" is a bit on the terse side?

Do you have some specific ideas about what one might say in addition or instead?

@pepe
Copy link
Member

pepe commented Apr 20, 2023

Position of the character in the string if possible? And thank you very much for trying to decipher my craps 😀

@sogaiu
Copy link
Contributor

sogaiu commented Apr 20, 2023

Ah I see. That might be nice.

Though if you look above the newly added code, I think you'll see that there are a couple of other error messages which are very much in the same style (i.e. no positional info) :)

there was a request to improve the error message, but the whole function
has non-informative errors. (both functions, actually, since the code is
duplicated)
as such, instead of catching it directly, address the assumption that
led to the SIGSEGV and let it be caught by the functions themselves,
thus reusing existing error messages (which can then be improved
separately).
@CosmicToast
Copy link
Contributor Author

It's rather complicated because the information on the string format location is partial and lost (see: the "% " example), as well as disparate between the two functions (one of them doesn't hold an intermediary pointer), which is why the existing error messages don't hold location information.
To make this a bit more useful, I addressed the source of the error instead of the symptom (i.e the strchr was checked against NULL, but not against returning the location of \0, which it can and should according to the specification).
This means that the error handling falls through into janet_formatbv and janet_buffer_format, which then fall through to the default case.
The new error messages look like so:

repl:1:> (printf "hello % " 10)
error: invalid conversion '% ' to 'format'
  in printf [src/core/io.c] on line 626
  in _thunk [repl] (tailcall) on line 1, column 1
repl:2:> (printf "hello %" 10)
error: invalid conversion '%' to 'format'
  in printf [src/core/io.c] on line 626
  in _thunk [repl] (tailcall) on line 2, column 1

This means that if a pass was ever done on those two functions (including to potentially deduplicate them), this case would benefit alongside all the other cases with location information.

@pepe
Copy link
Member

pepe commented Apr 20, 2023

Oh thank you both.

@CosmicToast CosmicToast changed the title check for NULL in get_fmt_mapping (fixes #1105) handle null-byte case in scanformat (fixes #1105) Apr 22, 2023
@bakpakin
Copy link
Member

Looks like a good fix to me right now, we could definitely improve some of the error handling in some cases, but I think it is more important to fix segfaults and things before worrying about the error case flow

@bakpakin bakpakin merged commit d9ed7a7 into janet-lang:master Apr 22, 2023
@pepe
Copy link
Member

pepe commented Apr 24, 2023

Indeed. I am sorry about the not needed discussion.

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.

4 participants