Assertions as binary data validation checks in deserialization

[Andrey Kuznetsov]

Hi Igniters,

While examining BinaryObjectImpl code I found this curious line in typeId()
method:

  assert arr[off] == GridBinaryMarshaller.STRING : arr[off];

Is it OK to check external binary data with assertions?
I think it can lead to undefined behaviour on corrupt data from the wire.

--
Best regards,
  Andrey Kuznetsov.

[Valentin Kulichenko]

Andrey,

How will it corrupt the data? Assertions only reads the array, not updates
it, right?

-Val

On Thu, Jul 27, 2017 at 8:54 AM, Andrey Kuznetsov <[hidden email]> wrote:

> Hi Igniters,
>
> While examining BinaryObjectImpl code I found this curious line in typeId()
> method:
>
>   assert arr[off] == GridBinaryMarshaller.STRING : arr[off];
>
> Is it OK to check external binary data with assertions?
> I think it can lead to undefined behaviour on corrupt data from the wire.
>
> --
> Best regards,
>   Andrey Kuznetsov.
>

[Andrey Kuznetsov]

Valentin,

I meant the behaviour of this code when corrupted data from the network are
being deserialized. Assertion is no-op in production, and we silently
ignore binary format violation.

27 июля 2017 г. 21:09 пользователь "Valentin Kulichenko" <
[hidden email]> написал:

> Andrey,
>
> How will it corrupt the data? Assertions only reads the array, not updates
> it, right?
>
> -Val
>
> On Thu, Jul 27, 2017 at 8:54 AM, Andrey Kuznetsov <[hidden email]>
> wrote:
>
> > Hi Igniters,
> >
> > While examining BinaryObjectImpl code I found this curious line in
> typeId()
> > method:
> >
> >   assert arr[off] == GridBinaryMarshaller.STRING : arr[off];
> >
> > Is it OK to check external binary data with assertions?
> > I think it can lead to undefined behaviour on corrupt data from the wire.
> >
> > --
> > Best regards,
> >   Andrey Kuznetsov.
> >
>

[Valentin Kulichenko]

Do you suggest to throw an exception instead of assertions?

-Val

On Thu, Jul 27, 2017 at 11:52 AM, Andrey Kuznetsov <[hidden email]>
wrote:

> Valentin,
>
> I meant the behaviour of this code when corrupted data from the network are
> being deserialized. Assertion is no-op in production, and we silently
> ignore binary format violation.
>
> 27 июля 2017 г. 21:09 пользователь "Valentin Kulichenko" <
> [hidden email]> написал:
>
> > Andrey,
> >
> > How will it corrupt the data? Assertions only reads the array, not
> updates
> > it, right?
> >
> > -Val
> >
> > On Thu, Jul 27, 2017 at 8:54 AM, Andrey Kuznetsov <[hidden email]>
> > wrote:
> >
> > > Hi Igniters,
> > >
> > > While examining BinaryObjectImpl code I found this curious line in
> > typeId()
> > > method:
> > >
> > >   assert arr[off] == GridBinaryMarshaller.STRING : arr[off];
> > >
> > > Is it OK to check external binary data with assertions?
> > > I think it can lead to undefined behaviour on corrupt data from the
> wire.
> > >
> > > --
> > > Best regards,
> > >   Andrey Kuznetsov.
> > >
> >
>

[Andrey Kuznetsov]

Indeed, "let it crash" approach is better than unclear error in some
indeterminate place later. Here we depend on data from "inpredictable"
source, so assertions are not suitable.

27 июля 2017 г. 22:35 пользователь "Valentin Kulichenko" <
[hidden email]> написал:

Do you suggest to throw an exception instead of assertions?

-Val

[Valentin Kulichenko]

Makes sense to me. Feel free to create a ticket unless someone else has any
objection. However, I think that we should revise other code for such
places then. Fixing only this one line doesn't change a lot.

-Val

On Thu, Jul 27, 2017 at 12:55 PM, Andrey Kuznetsov <[hidden email]>
wrote:

> Indeed, "let it crash" approach is better than unclear error in some
> indeterminate place later. Here we depend on data from "inpredictable"
> source, so assertions are not suitable.
>
> 27 июля 2017 г. 22:35 пользователь "Valentin Kulichenko" <
> [hidden email]> написал:
>
> Do you suggest to throw an exception instead of assertions?
>
> -Val
>

[Andrey Kuznetsov]

May I change this line while working on existing ticket, unless someone
else objects?

27 июля 2017 г. 23:37 пользователь "Valentin Kulichenko" <
[hidden email]> написал:

Makes sense to me. Feel free to create a ticket unless someone else has any
objection. However, I think that we should revise other code for such
places then. Fixing only this one line doesn't change a lot.

-Val

[Vladimir Ozerov]

We have a lot of such assertions. They are created for developers, not for
users. We cannot cover all such places with checks for corrupted data or
so, as it will decrease performance significantly.

Hence, proposed fix doesn't make sense to me.

чт, 27 июля 2017 г. в 23:43, Andrey Kuznetsov <[hidden email]>:

> May I change this line while working on existing ticket, unless someone
> else objects?
>
> 27 июля 2017 г. 23:37 пользователь "Valentin Kulichenko" <
> [hidden email]> написал:
>
> Makes sense to me. Feel free to create a ticket unless someone else has any
> objection. However, I think that we should revise other code for such
> places then. Fixing only this one line doesn't change a lot.
>
> -Val
>