Fix #63: do not panic into C #91

rekka · 2017-06-11T12:59:27Z

Instead of panicking on failure, add a warning and return an error value
(if possible).

Closes #63.

However, these kinds of problems should probably result in a failure since they indicate an internal engine error.

ronnychevalier · 2017-06-11T13:06:04Z

src/engines/mod.rs

+        _ => {
+                tt_warning!(es.status, "Unexpected \"whence\" parameter to fseek() wrapper: {}",
+                            whence);
+                return 0;


I think it would make more sense to change input_seek to return a signed value. fseek returns -1 and set errno to EINVAL when there is an invalid whence provided. I think it would make sense to return -EINVAL (e.g., ssize_t is signed). A negative value so that the caller knows there is an error and its value set to an error to know the reason if needed.

Thanks for the suggestion. Unfortunately, as far as can tell, the semantics of input_seek are not clear. It is not really a wrapper around fseek, so that message is a bit misleading. Looking at the history, it was used to replace various methods, mostly rewind, which does not report errors. In fact, the code does not check the return value, except at two places in dpx-pdfobj.c like this inefficient looking search for the beginning of the line. There it means the current position. I agree that it would be definitely better to abort the engine and report a proper error. I think that it will need a more serious refactoring of the bridge code. This commit at least fixes the undefined behavior.

Sure, maybe add a TODO or an issue to keep track of that? We should handle normal errors from seek and also the case of an unknown whence parameter.

Good point. I have also moved the error conversion from ExecutionState::input_seek to input_seek. I believe it is better to keep the conversion "Rust error" -> "C error" all in one place in the bridge function.

pkgw

Nice, thanks! Besides the small comments I had, can you change the tt_warning! macros to tt_error! calls?

pkgw · 2017-06-12T04:59:30Z

src/engines/mod.rs

@@ -383,7 +377,9 @@ impl<'a, I: 'a + IoProvider> ExecutionState<'a, I> {
            }
        }

-        panic!("unexpected handle {:?}", handle);
+        tt_warning!(self.status, "input close failed: unexpected handle {:?}", handle);


Can you add statement making it clearer that this is an internal bug? Something like "serious internal bug: unexpected handle in input close".

Do you want me to add "serious internal bug" only to this particular error, or to others as well?

pkgw · 2017-06-12T05:01:35Z

src/engines/mod.rs

-        _ => panic!("Unexpected \"whence\" parameter to fseek() wrapper: {}", whence),
+        _ => {
+                // TODO: Handle the error better. This indicates a bug in the engine.
+                tt_warning!(es.status, "Unexpected \"whence\" parameter to fseek() wrapper: {}",


Very minor, but while we're here, I'm trying to be more consistent about these sorts of messages not starting with capital letters.

This is also an internal error, maybe also add "serious internal error"?

rekka · 2017-06-12T11:58:51Z

Thanks for the review. Should I change just the tt_warnings that this commit touches, or should I do it with the other tt_warnings throughout the file?

It might be a good idea to unify the format of all the internal errors in this file when I am already doing the edits.

pkgw · 2017-06-12T17:51:39Z

I'll trust your judgment on both questions. For things like the "whence" problem, I think it is good to somehow try to convey to users that the program has some crazy problem and, importantly, that they (the users) will almost certainly not be able to fix it themselves. In fact, it might be smart to add some infrastructure for "internal bug" type problems that automatically produces a message to this effect, telling the user to visit the issue tracker.

As for tt_warning to tt_error change, the distinction that I try to maintain is whether the output file is probably going to be fine, or it's probably not going to be fine. For these kinds of problems that represent a failure of internal consistency, I think it is better to assume that the output will probably not be fine ⇒ those sorts of problems should be classified as errors.

rekka · 2017-06-13T07:18:36Z

I see, thanks! I'll try to do something reasonable.

pkgw · 2017-08-28T14:17:21Z

Hi @rekka,

Sorry for being so inactive on this. I'd like to get this PR actually merged! If you have a chance to make the final changes I suggested that would be great. Otherwise do you mind if I extend your PR with a couple of commits of my own that make the changes?

rekka · 2017-08-29T04:07:15Z

Hi @pkgw,

Sorry about not making progress on this. OK, I will try to have a look in the next few days and finish the pull request.

rekka · 2017-10-02T06:44:49Z

Hi, just to give an update, I haven't yet been able to spend much time on this because of a few deadlines that started coming up. If you prefer to get this merged soon, feel free to edit the PR. Sorry about that...

Instead of panicking on failure, add a warning and return an error value (if possible). Closes tectonic-typesetting#63. - Move error conversion to the `input_seek` wrapper. - Add TODOs in `input_seek` to handle the serious internal errors properly. Optimally, tectonic should stop when an internal error is encountered in seek.

codecov · 2018-08-24T03:32:41Z

Codecov Report

Merging #91 into master will decrease coverage by <.01%.
The diff coverage is 72%.

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
- Coverage   37.88%   37.88%   -0.01%     
==========================================
  Files         131      131              
  Lines       61500    61523      +23     
==========================================
+ Hits        23298    23306       +8     
- Misses      38202    38217      +15

Impacted Files	Coverage Δ
src/engines/mod.rs	`72.61% <70.58%> (-1.71%)`	⬇️
tectonic/core-bridge.c	`72.13% <75%> (+1.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 091fdac...daeb229. Read the comment docs.

rekka · 2018-08-24T03:47:58Z

Sorry about the delay! I think it's time to fix this possible UB.

I added some crude handling of the internal bug when calling input_seek and input_close by aborting the engine using longjmp.

I also checked the other conditions that generate tt_warning in the bridge API and they all do not seem very serious so a warning is appropriate. They are basically related to IO and not caused by tectonic's bridge API, and the errors are reported by the return value back to xetex. Though the xetex engine is not very consistent at checking the error conditions.

I noticed that all the functions in engines/mod.rs that are called from C are missing the extern keyword. This seems like a bug since Rust's ABI is not stable and not necessary compatible with C rust-lang/rfcs#600. Let me know if you agree and I'll add the keyword to the declarations.

pkgw · 2018-08-24T14:40:40Z

Yes, based on what I can find, it sounds like it is correct to add extern "C" to the callbacks that are passed to the C code. Good catch!

edit: link for posterity

rekka · 2018-08-24T23:17:18Z

I have added the extern keywords. The use without "C" seems to be more idiomatic https://github.com/alexcrichton/rust-ffi-examples/blob/a2ddf21e48b8c907d5a049114698283c2cca7847/c-to-rust/src/lib.rs.

rekka · 2018-08-27T05:04:45Z

#188 is a panic across FFI boundary as well, triggering undefined behavior. Surprisingly, the stack trace works even for the C functions...

We either should be careful not to panic in any Rust functions called by the C code, or wrap everything in catch_unwind.

pkgw · 2018-09-10T12:34:22Z

Sorry for taking so long to merge this!

I think I read somewhere that longjmp in C code called by Rust is not guaranteed to work, which will be quite a bummer if it ever starts causing problems ...

rekka · 2018-09-11T01:23:49Z

Thank for merging.

And thanks for the comment about longjmp. After a bit of googling I found two references to a bug in rlua crate: rust-lang/rust#48251 and https://blog.rust-lang.org/2018/03/01/Rust-1.24.1.html. My understanding is that setjmp/longjmp that is contained within a C code and does not jump over Rust stack frames should work just fine. Tectonic stack frames are basically

Tectonic engine driver (Rust), which calls
Xetex engine (C), which calls
Tectonic IO routines (Rust)

setjmp is called on entering the Xetex engine, and longjmp can possibly only be called from the C code after returning from the tectonic IO routines. As all setjmp/longjmp logic is contained strictly in the C stack frames, I believe this is OK.

remove unused `mfgets`

ronnychevalier reviewed Jun 11, 2017

View reviewed changes

pkgw reviewed Jun 12, 2017

View reviewed changes

rekka added 2 commits August 24, 2018 10:37

Abort the engine on internal error in input_close

26d99e0

rekka force-pushed the fix-63 branch from 7533dca to 26d99e0 Compare August 24, 2018 01:57

Abort the engine on internal error in input_seek

b20f34d

Add extern keyword to functions called from C

daeb229

pkgw self-assigned this Sep 10, 2018

pkgw merged commit 7f3326e into tectonic-typesetting:master Sep 10, 2018

rekka deleted the fix-63 branch August 30, 2019 10:51

Mrmaxmeier pushed a commit to Mrmaxmeier/tectonic that referenced this pull request Oct 1, 2019

Merge pull request tectonic-typesetting#91 from burrbull/fileio

aab449a

remove unused `mfgets`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #63: do not panic into C #91

Fix #63: do not panic into C #91

rekka commented Jun 11, 2017

ronnychevalier Jun 11, 2017

rekka Jun 11, 2017

ronnychevalier Jun 11, 2017

rekka Jun 11, 2017

pkgw left a comment

pkgw Jun 12, 2017

rekka Jun 12, 2017

pkgw Jun 12, 2017

rekka Jun 12, 2017

rekka commented Jun 12, 2017 •

edited

Loading

pkgw commented Jun 12, 2017 •

edited

Loading

rekka commented Jun 13, 2017

pkgw commented Aug 28, 2017

rekka commented Aug 29, 2017

rekka commented Oct 2, 2017

codecov bot commented Aug 24, 2018 •

edited

Loading

rekka commented Aug 24, 2018

pkgw commented Aug 24, 2018 •

edited

Loading

rekka commented Aug 24, 2018

rekka commented Aug 27, 2018

pkgw commented Sep 10, 2018

rekka commented Sep 11, 2018

Fix #63: do not panic into C #91

Fix #63: do not panic into C #91

Conversation

rekka commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkgw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rekka commented Jun 12, 2017 • edited Loading

pkgw commented Jun 12, 2017 • edited Loading

rekka commented Jun 13, 2017

pkgw commented Aug 28, 2017

rekka commented Aug 29, 2017

rekka commented Oct 2, 2017

codecov bot commented Aug 24, 2018 • edited Loading

Codecov Report

rekka commented Aug 24, 2018

pkgw commented Aug 24, 2018 • edited Loading

rekka commented Aug 24, 2018

rekka commented Aug 27, 2018

pkgw commented Sep 10, 2018

rekka commented Sep 11, 2018

rekka commented Jun 12, 2017 •

edited

Loading

pkgw commented Jun 12, 2017 •

edited

Loading

codecov bot commented Aug 24, 2018 •

edited

Loading

pkgw commented Aug 24, 2018 •

edited

Loading