Fix segfault in static_show, by using correct `vt` instead of `typeof(v)` #38399

NHDaly · 2020-11-11T18:48:37Z

This fixes a segfault introduced in #38049.

Co-Authored-By: @Sacha0

…(v)` This fixes a segfault introduced in JuliaLang#38049. Co-Authored-By: Sacha Verweij <sacha.verweij@alumni.stanford.edu>

src/rtutils.c

KristofferC · 2020-11-11T21:28:10Z

How often does this happen (aka how bad is it for PackageCompiler)? Also, can it be tested?

Sacha0 · 2020-11-11T22:07:05Z

How often does this happen (aka how bad is it for PackageCompiler)?

Every build on our end, regrettably.

Implement custom iterative check via `->super`

NHDaly · 2020-11-13T05:36:27Z

src/rtutils.c

+// explained in the comment on `jl_static_show_x_()`, below.
+// This function checks if `vt <: Function` without triggering GC.
+static int jl_static_is_function_(jl_datatype_t *vt) {
+    while (vt != jl_any_type) {


Is this sufficient stopping criteria to make sure we never infinite loop? Should I also check for NULL, for example?

Yes, this is fine.

because of where this gets called from, yeah, you should probably check for NULL also

Yeah, I was just about to come back and say that. This function is regularly called on invalid data. You might also want to include a max trip count or something in case the datatype loops.

Oh, shoot make sense.

Okay, i'll switch to print a warning. Thanks @yuyichao!

Sorry, one more question:

jl_printf((JL_STREAM*)STDERR_FILENO, or jl_printf(JL_STDERR,?

I see both examples in this file. I've gone with the first one, since it seems to be used in the jl_static_* functions?

Done (for now) in 7e2d967:

julia> ccall(:jl_static_show, Cvoid, (Ptr{Cvoid}, Any), p.in, x) ERROR: static_show: Exit after 10,000 iterations of supertype(). Presuming invalid cycle in datatype object.

(JL_STREAM*)STDERR_FILENO should be used. The use of JL_STDERR higher up in the file are not in the same context. As Keno said, this function needs to work as much as possible even on invalid data for debugging purpose. Reading of invalid memory is actually fine since there's protection agains it. Write of heap memory or use of julia runtime functions or external libraries (e.g. libuv) must be avoided.

Personally, I would just use jl_safe_printf since that function is basically reserved for this purpose. Alternatively, you can print something (or let the caller do so) to the current output stream, which should be safe as well. Simply ignoring the error is also a safe option and that's how jl_static_show_x_ handles clearly invalid tags.

Thinking about it more, I think I personally actually prefer to just ignore the error here. For valid objects, I don't think there is likely any false positive (and if it does the warning printed is wrong anyway...) so the error here doesn't really matter much.
OTOH, when inspecting a heap containing invalid objects, it'll be quite annoy if the printing is constantly interrupted by errors telling me something I already know about (i.e. invalid heap reference....). The only case I see this useful is to detect invalid types when the printing of it somehow still works. While that's not unimaginable, I don't think it's likely and this function isn't really the right place to do such a detection anyway...... We could have a function that walks the heap to detect such inconsistency if we really want to get this info...

FWIW, the rest of this code also used to use an iteration maximum, then print •, but it was removed in favor of explicit tracking with struct recur_list *depth. The helper functions, such as this one, first_arg_datatype and jl_static_show_func_sig have previously ignored those invalid possibilities.

Hrmm those are good points...

@vtjnash - without the check for infinite loops, the above example definitely does infinite loop:

julia> ccall(:jl_static_show, Cvoid, (Ptr{Cvoid}, Any), p.in, x) ^C^C^C^C^CWARNING: Force throwing a SIGINT ERROR: ^C^C^C^C^C^CInterruptException: Stacktrace: [1] top-level scope @ ./REPL[7]:1

Is there some way that I should use this recur_list *depth? I'm not exactly sure what you're recommending based on the above insight.

Based on the feedback from above, my current thinking is that I'll just remove the warnings, and return false from this function if we encounter a loop or a NULL, since both of those mean: it's not a function!

Does that sound good to you all?

I've pushed up aa77e35 which makes the above change. Please take another look if you can! :)

Since throwing an exception would allocate, which is illegal from the `jl_static_*` function(s). ``` julia> ccall(:jl_static_show, Cvoid, (Ptr{Cvoid}, Any), p.in, x) ERROR: static_show: Exit after 10,000 iterations of supertype(). Presuming invalid cycle in datatype object. ```

Sacha0

LGTM insofar as I can assess this! :)

yuyichao

......... I wrote this 3 hours ago and just noticed that I didn't submit the review = = ........
I'm still failing for the submit review vs submit single comment now = = ....

yuyichao · 2020-11-16T21:56:26Z

src/rtutils.c

+// explained in the comment on `jl_static_show_x_()`, below.
+// This function checks if `vt <: Function` without triggering GC.
+static int jl_static_is_function_(jl_datatype_t *vt) {
+    while (vt != jl_any_type) {


(JL_STREAM*)STDERR_FILENO should be used. The use of JL_STDERR higher up in the file are not in the same context. As Keno said, this function needs to work as much as possible even on invalid data for debugging purpose. Reading of invalid memory is actually fine since there's protection agains it. Write of heap memory or use of julia runtime functions or external libraries (e.g. libuv) must be avoided.

Personally, I would just use jl_safe_printf since that function is basically reserved for this purpose. Alternatively, you can print something (or let the caller do so) to the current output stream, which should be safe as well. Simply ignoring the error is also a safe option and that's how jl_static_show_x_ handles clearly invalid tags.

Thinking about it more, I think I personally actually prefer to just ignore the error here. For valid objects, I don't think there is likely any false positive (and if it does the warning printed is wrong anyway...) so the error here doesn't really matter much.
OTOH, when inspecting a heap containing invalid objects, it'll be quite annoy if the printing is constantly interrupted by errors telling me something I already know about (i.e. invalid heap reference....). The only case I see this useful is to detect invalid types when the printing of it somehow still works. While that's not unimaginable, I don't think it's likely and this function isn't really the right place to do such a detection anyway...... We could have a function that walks the heap to detect such inconsistency if we really want to get this info...

src/rtutils.c

If we ecounter a cycle or a NULL super type before encountering a Function super type, then this type is not <: Function.

NHDaly

Oops, i always forget to click re-request review.

Review re-requested. PTAL thanks!

NHDaly · 2020-11-20T15:16:37Z

Thanks! :)

NHDaly · 2020-11-24T16:39:01Z

If/when we do a 1.5.4 release, this should be included in it. Please let us know if we can help with that in any way!

…(v)` (#38399) This fixes a segfault introduced in #38049. (cherry picked from commit b602577)

Fix segfault in static_show, by using correct vt instead of `typeof…

8f54d34

…(v)` This fixes a segfault introduced in JuliaLang#38049. Co-Authored-By: Sacha Verweij <sacha.verweij@alumni.stanford.edu>

NHDaly mentioned this pull request Nov 11, 2020

segfault on Julia v1.5.3 JuliaLang/PackageCompiler.jl#466

Closed

vtjnash reviewed Nov 11, 2020

View reviewed changes

src/rtutils.c Outdated Show resolved Hide resolved

Fix illegal GC-violating call to jl_subtype in static_show

2e5f22c

Implement custom iterative check via `->super`

NHDaly commented Nov 13, 2020

View reviewed changes

NHDaly added 3 commits November 13, 2020 00:41

PR feedback: JL_NOTSAFEPOINT, jl_function_type

81aa68a

Add error checking to jl_static_is_function_: null and cycles

2479af7

Sacha0 approved these changes Nov 17, 2020

View reviewed changes

yuyichao reviewed Nov 17, 2020

View reviewed changes

vtjnash reviewed Nov 17, 2020

View reviewed changes

src/rtutils.c Outdated Show resolved Hide resolved

Remove WARNING logs on jl_static_is_function_()

aa77e35

If we ecounter a cycle or a NULL super type before encountering a Function super type, then this type is not <: Function.

NHDaly requested review from vtjnash, yuyichao and Keno November 19, 2020 19:09

NHDaly commented Nov 19, 2020

View reviewed changes

vtjnash merged commit b602577 into JuliaLang:master Nov 19, 2020

NHDaly deleted the static_show-segfault-fix branch November 20, 2020 15:16

StefanKarpinski added the backport 1.5 label Nov 24, 2020

KristofferC mentioned this pull request Dec 5, 2020

Segfault with PackageCompiler and Unitful on Julia v1.5 (regression) #38719

Closed

vchuravy mentioned this pull request Jan 21, 2021

Backports 1.5.4 #39351

Merged

27 tasks

vchuravy pushed a commit that referenced this pull request Jan 21, 2021

Fix segfault in static_show, by using correct vt instead of `typeof…

fd3af72

…(v)` (#38399) This fixes a segfault introduced in #38049. (cherry picked from commit b602577)

Sacha0 removed the backport 1.5 label Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix segfault in static_show, by using correct `vt` instead of `typeof(v)` #38399

Fix segfault in static_show, by using correct `vt` instead of `typeof(v)` #38399

NHDaly commented Nov 11, 2020 •

edited

Loading

KristofferC commented Nov 11, 2020

Sacha0 commented Nov 11, 2020

NHDaly Nov 13, 2020

Keno Nov 13, 2020

NHDaly Nov 13, 2020

vtjnash Nov 13, 2020

Keno Nov 13, 2020

NHDaly Nov 16, 2020

yuyichao Nov 16, 2020

vtjnash Nov 17, 2020

NHDaly Nov 18, 2020

NHDaly Nov 18, 2020

Sacha0 left a comment

yuyichao left a comment

yuyichao Nov 16, 2020

NHDaly left a comment

NHDaly commented Nov 20, 2020

NHDaly commented Nov 24, 2020

Fix segfault in static_show, by using correct vt instead of typeof(v) #38399

Fix segfault in static_show, by using correct vt instead of typeof(v) #38399

Conversation

NHDaly commented Nov 11, 2020 • edited Loading

KristofferC commented Nov 11, 2020

Sacha0 commented Nov 11, 2020

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

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

Sacha0 left a comment

Choose a reason for hiding this comment

yuyichao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NHDaly left a comment

Choose a reason for hiding this comment

NHDaly commented Nov 20, 2020

NHDaly commented Nov 24, 2020

Fix segfault in static_show, by using correct `vt` instead of `typeof(v)` #38399

Fix segfault in static_show, by using correct `vt` instead of `typeof(v)` #38399

NHDaly commented Nov 11, 2020 •

edited

Loading