Eli Zaretskii schrieb am So., 1. Okt. 2017 um 20:06 Uhr: > > From: Philipp Stephani > > Date: Sat, 30 Sep 2017 22:02:55 +0000 > > Cc: emacs-devel@gnu.org > > > > Subject: [PATCH] Implement native JSON support using Jansson > > Thanks, a few more comments/questions. > > > +#if __has_attribute (warn_unused_result) > > +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ > ((__warn_unused_result__)) > > +#else > > +# define ATTRIBUTE_WARN_UNUSED_RESULT > > +#endif > > Hmm... why do we need this attribute? You use it with 2 static > functions, so this sounds like a left-over from the development stage? > It's not strictly needed (and if you don't like it, I can remove it), but it helps catching memory leaks. > > > +static Lisp_Object > > +internal_catch_all_1 (Lisp_Object (*function) (void *), void *argument) > > Can you tell why you needed this (and the similar internal_catch_all)? > Is that only because the callbacks could signal an error, or is there > another reason? If the former, I'd prefer to simplify the code and > its maintenance by treating the error condition in a less drastic > manner, and avoiding the call to xsignal. > The callbacks (especially insert and before-/after-change-hook) can exit nonlocally, but these nonlocal exits may not escape the Jansson callback. Therefore all nonlocal exits must be caught here. > > > +static _GL_ARG_NONNULL ((2)) Lisp_Object > > +lisp_to_json_toplevel_1 (Lisp_Object lisp, json_t **json) > > +{ > > + if (VECTORP (lisp)) > > + { > > + ptrdiff_t size = ASIZE (lisp); > > + eassert (size >= 0); > > + if (size > SIZE_MAX) > > + xsignal1 (Qoverflow_error, build_string ("vector is too long")); > > I think this error text is too vague. Can we come up with something > that describes the problem more accurately? Maybe, but it's probably not worth it because I don't think we have many architectures where PTRDIFF_MAX > SIZE_MAX. > > And btw, how can size be greater than SIZE_MAX in this case? This is > a valid Lisp object, isn't it? (There are more such tests in the > patch, e.g. in lisp_to_json, and I think they, too, are redundant.) > Depends on the range of ptrdiff_t and size_t. IIUC nothing in the C standard guarantees PTRDIFF_MAX <= SIZE_MAX. If we want to guarantee that, we should probably add at least a static assertion. > > > + *json = json_check (json_array ()); > > + ptrdiff_t count = SPECPDL_INDEX (); > > + record_unwind_protect_ptr (json_release_object, json); > > + for (ptrdiff_t i = 0; i < size; ++i) > > + { > > + int status > > + = json_array_append_new (*json, lisp_to_json (AREF (lisp, > i))); > > + if (status == -1) > > + json_out_of_memory (); > > + eassert (status == 0); > > + } > > + eassert (json_array_size (*json) == size); > > + clear_unwind_protect (count); > > + return unbind_to (count, Qnil); > > This, too, sounds more complex than it should: you record > unwind-protect just so lisp_to_json's subroutines could signal an > error due to insufficient memory, right? Why can't we have the > out-of-memory check only inside this loop, which you already do, and > avoid the checks on lower levels (which undoubtedly cost us extra > cycles)? What do those extra checks in json_check buy us? the errors > they signal are no more informative than the one in the loop, AFAICT. > I don't understand what you mean. We need to check the return values of all functions if we want to to use them later. > > > +static Lisp_Object > > +json_insert (void *data) > > +{ > > + const struct json_buffer_and_size *buffer_and_size = data; > > + if (buffer_and_size->size > PTRDIFF_MAX) > > + xsignal1 (Qoverflow_error, build_string ("buffer too large")); > > + insert (buffer_and_size->buffer, buffer_and_size->size); > > I don't think we need this test here, as 'insert' already has the > equivalent test in one of its subroutines. > It can't, because it takes the byte length as ptrdiff_t. We need to check before whether the size is actually in the valid range of ptrdiff_t. > > > + case JSON_INTEGER: > > + { > > + json_int_t value = json_integer_value (json); > > + if (FIXNUM_OVERFLOW_P (value)) > > + xsignal1 (Qoverflow_error, > > + build_string ("JSON integer is too large")); > > + return make_number (value); > > This overflow test is also redundant, as make_number already does it. > It can't, because json_int_t can be larger than EMACS_INT. Also, make_number doesn't contain any checks. > > > + case JSON_STRING: > > + { > > + size_t size = json_string_length (json); > > + if (FIXNUM_OVERFLOW_P (size)) > > + xsignal1 (Qoverflow_error, build_string ("JSON string is too > long")); > > + return json_make_string (json_string_value (json), size); > > Once again, the overflow test is redundant, as make_specified_string > (called by json_make_string) already includes an equivalent test. > And once again, we need to check at least whether the size fits into ptrdiff_t. > > > + case JSON_ARRAY: > > + { > > + if (++lisp_eval_depth > max_lisp_eval_depth) > > + xsignal0 (Qjson_object_too_deep); > > + size_t size = json_array_size (json); > > + if (FIXNUM_OVERFLOW_P (size)) > > + xsignal1 (Qoverflow_error, build_string ("JSON array is too > long")); > > + Lisp_Object result = Fmake_vector (make_natnum (size), > Qunbound); > > Likewise here: Fmake_vector makes sure the size is not larger than > allowed. > Same as above: It can't. > > > + case JSON_OBJECT: > > + { > > + if (++lisp_eval_depth > max_lisp_eval_depth) > > + xsignal0 (Qjson_object_too_deep); > > + size_t size = json_object_size (json); > > + if (FIXNUM_OVERFLOW_P (size)) > > + xsignal1 (Qoverflow_error, > > + build_string ("JSON object has too many elements")); > > + Lisp_Object result = CALLN (Fmake_hash_table, QCtest, Qequal, > > + QCsize, make_natnum (size)); > > Likewise here: make_natnum does the equivalent test. > It doesn't and can't. > > > + /* Adjust point by how much we just read. Do this here because > > + tokener->char_offset becomes incorrect below. */ > > + bool overflow = INT_ADD_WRAPV (point, error.position, &point); > > + eassert (!overflow); > > + eassert (point <= ZV_BYTE); > > + SET_PT_BOTH (BYTE_TO_CHAR (point), point); > > It's better to use SET_PT here, I think. > That's not possible because we don't have the character offset. (And I think using SET_PT (BYTE_TO_CHAR (point)) would just require needlessly recalculating point.) > > > + define_error (Qjson_out_of_memory, "no free memory for creating JSON > object", > > I'd prefer "not enough memory for creating JSON object". > > Done.