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

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

Merged
merged 6 commits into from
Nov 19, 2020
Merged
26 changes: 25 additions & 1 deletion src/rtutils.c
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,30 @@ static size_t jl_static_show_x_sym_escaped(JL_STREAM *out, jl_sym_t *name) JL_NO
return n;
}

// `jl_static_show()` cannot call `jl_subtype()`, for the GC reasons
// 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) JL_NOTSAFEPOINT {
if (!jl_function_type) { // Make sure there's a Function type defined.
return 0;
}
int _iter_count = 0; // To prevent infinite loops from corrupt type objects.
while (vt != jl_any_type) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks! :)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

(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...

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if (vt == NULL) {
return 0;
} else if (_iter_count > 10000) {
// We are very likely stuck in a cyclic datastructure, so we assume this is
// _not_ a Function.
return 0;
} else if (vt == jl_function_type) {
return 1;
}
vt = vt->super;
_iter_count += 1;
}
return 0;
}

// `v` might be pointing to a field inlined in a structure therefore
// `jl_typeof(v)` may not be the same with `vt` and only `vt` should be
// used to determine the type of the value.
Expand Down Expand Up @@ -999,7 +1023,7 @@ static size_t jl_static_show_x_(JL_STREAM *out, jl_value_t *v, jl_datatype_t *vt
n += jl_static_show_x(out, *(jl_value_t**)v, depth);
n += jl_printf(out, ")");
}
else if (jl_function_type && jl_isa(v, (jl_value_t*)jl_function_type)) {
else if (jl_static_is_function_(vt)) {
// v is function instance (an instance of a Function type).
jl_datatype_t *dv = (jl_datatype_t*)vt;
jl_sym_t *sym = dv->name->mt->name;
Expand Down