[vlc-devel] [PATCH] stream_ReadLine: increase line length limit

[Pierre Ynard]

Thank you for your reply. As I explained in:

https://trac.videolan.org/vlc/ticket/24957

It's not realistically possible to avoid buffering very long lines
or text blobs into single strings of arbitrary length. If not in
stream_ReadLine(), it will have to be done somewhere else anyway (and
probably more poorly).

I'm not going to rewrite lua scripts under a new paradigm different from
the current line-based one just for that: I'm not going to rewrite them
as XML parsers (which isn't even currently possible), nor reimplement
in lua the same stream_ReadLine() buffering in a half-assed way.
Copy-pasting from the ticket:

> So I think the best solutions are:
>
> 1. Bumping up the current limit in the stream code. Even though it's
>    outside the scope of lined-base file formats that you assert for
>    this function, it causes no issue or downside, doesn't add
>    complexity, and expands support.
>
> 2. Copy-pasting the vlc_stream_ReadLine() code into the lua module,
>    and enabling long line support there, since that implementation
>    will definitely include parsing HTML, JSON and web API data within
>    its scope. This still seems absurd to me though.
>
> 3. Leaving YouTube playback broken and getting more and more broken.
>    Users seem to find this feature pretty popular though.

You've NACKed 1. I'm not going to do 2 because I think it's stupid, and
it's not exactly as simple as copy-pasting the code, and I don't have
a VLC development environment now. So I guess it's going to be 3, and
I'm going to wash my hands of it. Maybe I'll just cook some unshareable
fix for my own system, if possible. Someone complained by email already
on Monday, that YouTube playback was broken; and who knows what he came
across exactly, but I suspect that the sporadic failures I've been
noticing over the past months but had been unable to reproduce, are now
ramping up.

> Nack. The limit is already insanely high considering the use cases,
> simple playlist and subtitle formats. We don't want to inject 4 MiB
> URLs.

I can't see any documentation in the code saying that the
vlc_stream_ReadLine() utility is specifically for simple playlist and
subtitle formats, rather than a generic text stream utility; but those
are certainly valid use cases. If you think it's important and valuable
to protect those from very long lines, I can still offer more solutions
than above:

 - changing the API to add an optional parameter to override this
 - introducing a VLC object variable to control that behavior, that
   could be set from the lua stream filter
 - moving the line length safety check somewhere else, into a code path
   specific to playlists and subtitles

But I'm not going to poke in the dark for ways to skirt an API's
restrictions, that you likely won't find acceptable anyway.

If you think rejecting very long lines is not just some theoretical
safety net that shouldn't be encountered, but something intended to
happen, on purpose and by design, then you should submit a patch that
prints an error message owning that behavior, rather than failing
silently.

> Nack. The limit is already insanely high considering the use cases,
> simple playlist and subtitle formats.

This implies that the whole lua playlist script feature is not a valid
use case of one of the main API it relies on. So either it was based
from the start on an API abuse and then further built upon it to the
current extent, or despite its current extent you deny its de facto
status as a use case. Either way, that's pretty messed up.

Do you think the whole lua stack should be scrapped? If you think
the whole lua stack should be scrapped, then submit the idea, gather
a consensus, and set up a migration plan and a timeline for it. If
you think it should be kept, then don't treat it as an invalid or
second-rate use case, not worthy of getting fully supported by core
subsystems.

As for another main lua feature that's also getting challenged, the
default CLI in 4.0 can and in my opinion should be re-reverted to the
lua CLI. The C CLI still lacks important features, the reversion causes
still unaddressed regression issues, and more importantly goes counter
to architectural vision.

-- 
Pierre Ynard
"Une âme dans un corps, c'est comme un dessin sur une feuille de papier."