From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Philipp Stephani
On 10/03/2017 05:26 AM, Philipp Stephani wrote:
> > +#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?=C2=A0 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 remo= ve it),
> but it helps catching memory leaks.
I've found __warn_unused_result__ to be more trouble than it's wort= h in
library functions. Emacs has lib/ignore-value.h in order to work around
__warn_unused_result__ brain damage, for example. For static functions
the problem is less, but still, I mildly favor leaving it out.OK, I'll remove it.=C2=A0
>
>=C2=A0 =C2=A0 =C2=A0And btw, how can size be greater than SIZE_MAX in t= his case? This is
>=C2=A0 =C2=A0 =C2=A0a valid Lisp object, isn't it?=C2=A0 (There are= more such tests in the
>=C2=A0 =C2=A0 =C2=A0patch, 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 <=3D SIZE_MAX. If we want to guaran= tee
> that, we should probably add at least a static assertion.
There should be no need for that. No Lisp object can exceed min
(PTRDIFF_MAX, SIZE_MAX) bytes; alloc.c guarantees this, so that Emacs
should work even on oddball platforms where SIZE_MAX < PTRDIFF_MAX, and<= br> there is no need for a runtime check here.
> > +=C2=A0 if (buffer_and_size->size > PTRDIFF_MAX)
> > +=C2=A0 =C2=A0 xsignal1 (Qoverflow_error, build_string ("buf= fer too large"));
> > +=C2=A0 insert (buffer_and_size->buffer, buffer_and_size->s= ize);
>
> 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 t= o
> check before whether the size is actually in the valid range of ptrdif= f_t.
A PTRDIFF_MAX test is needed if the JSON library can return strings
longer than PTRDIFF_MAX. Please just use buffer_overflow () to signal
the error, though.
> > +=C2=A0 =C2=A0 case JSON_INTEGER:
> > +=C2=A0 =C2=A0 =C2=A0 {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 json_int_t value =3D json_integer_va= lue (json);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (FIXNUM_OVERFLOW_P (value))
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 xsignal1 (Qoverflow_error,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 build_string ("JSON integer is too large"));
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return make_number (value);
>
> This overflow test is also redundant, as make_number already does it.<= br> >
> It can't, because json_int_t can be larger than EMACS_INT. Also, > make_number doesn't contain any checks.
You're right that a test is needed. However, elsewhere we just use
xsignal0 (Qoverflow_error) for this sort of thing, and I suggest being
consistent and doing that here as well. Similarly for other calls to
xsignal1 with Qoverflow_error.
> > +=C2=A0 =C2=A0 case JSON_STRING:
> > +=C2=A0 =C2=A0 =C2=A0 {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 size_t size =3D json_string_length (= json);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (FIXNUM_OVERFLOW_P (size))
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 xsignal1 (Qoverflow_error, bu= ild_string ("JSON string is
> too long"));
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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.
You're right, a test is needed. However, I suggest using string_overflo= w
() to signal string overflows.