SageTV Service and Client not behaving

[sdsean]

They are all from IMDB saves via CMT, I have no doubt that there are likely several that are very large. Let me try removing imported dirs as you suggest, were they all imports or did you see that in recordings?

[sdsean]

Strangely though I don't think the large description issue (unless it was on recordings) is what's causing a corrupt wiz.bin / fallback since it always seems to happen when I try to reestablish my favorites (though I could be wrong).

I'm moving out all my recordings and also gonna try removing imports after i test each time re-establishing favorites. Let me know if you guys see anything else in the logs.

[wnjj]

Originally Posted by sdsean

Strangely though I don't think the large description issue (unless it was on recordings) is what's causing a corrupt wiz.bin / fallback since it always seems to happen when I try to reestablish my favorites (though I could be wrong).

I'm moving out all my recordings and also gonna try removing imports after i test each time re-establishing favorites. Let me know if you guys see anything else in the logs.

The too-large description is exactly why it's crashing and "corrupting" wiz.bin. The negative "size" is causing a Java exception when trying to allocate space for the description data (line 475 in Show.Java). It's using the "size" value it read in from the file which is negative. That negative value happened when it previously wrote wiz.bin with a size > 32k and overflowed the short int.

[sdsean]

OK I see now what you're saying, sure would be helpful if it gave you the show id / text or something. Anyway I'm going to start unimporting things and deleting properties files

[wnjj]

Originally Posted by sdsean

OK I see now what you're saying, sure would be helpful if it gave you the show id / text or something. Anyway I'm going to start unimporting things and deleting properties files

If you still have one of the corrupt wiz.bin's, trying running it through my Wiz2SQL tool (https://forums.sagetv.com/forums/showthread.php?t=62512). I don't know what will happen when my tool encounters a negative number but I could always tweak a version for you to try that will halt and spit out debug info if it finds an "illegal" desc length.

[wnjj]

Looks like Wiz2SQL reads the "short" value into an int internally so will likely read it all just fine. You can then search for "long" descriptions using a text script language of your choice...or I could add a feature to Wiz2SQL to flag these "oversized" short values.

[sdsean]

sweet i'll try that, I'm hoping that it's a "show" that's the problem since it's in Show.java (though I suppose it could be any media). since i have no recordings hooked up at the moment that may solve it. . .

however if indeed it is a recording then that means the import from schedulesdirect needs some help (or maybe CMT again there) since the code is clearly not truncating it. . .

where do i file a bug? just in the issues on github?

[sdsean]

You know, I'm reading this code and I'm baffled as to how the application is getting into this state. The only place in Show.java where descBytes is set is here:
https://github.com/google/sagetv/blo...java#L471-L476

And here:
https://github.com/google/sagetv/blo...Show.java#L361

So I guess it must be coming from a Show::update call since the other place reads everything as a short?

[JustFred]

No argument that Sage should handle the huge descriptions without crashing. But it's also arguable that whatever is creating them (CMT? BMT?) needs to keep things under control, too.

Looking at the logs, there are quite a few imported videos with metadata that is HUGE. In the case of Thor: Ragnarok, it appears that most (all?) of the 1,223 User Reviews have been imported. Is that really what was intended?

It'll probably be easy to spot this kind of nonsense by simply looking at the file size of the .properties files for each of the imports. Anything near 32K or larger is suspect. Apart from the corruption of wiz.bin, it's anybody's guess what impact these huge property files have on the utilization of Java's heap space.

[sdsean]

Well I have a few dozen .properties files that are over 32KB, granted that in and of itself is not necessarily a problem since a prop file consists of more than just the description, but as you noted Ragnarok is one that's over 200KB, ForceAwakens in my library is another that's ginormous.

I have a PR ready to go that will sniff out descriptions longer than 32KB and then force it into string mode. . . .though that's only on write and this appears to be on a read that's actually causing the problem (as in an update occurs from say a prop file and in turn the byte array is too big) so i'm not sure that my PR would actually solve the problem. . .

That said I'm going to delete really big prop files just to be safe (anything over 64KB)

[wnjj]

Originally Posted by sdsean

You know, I'm reading this code and I'm baffled as to how the application is getting into this state. The only place in Show.java where descBytes is set is here:
https://github.com/google/sagetv/blo...java#L471-L476

And here:
https://github.com/google/sagetv/blo...Show.java#L361

So I guess it must be coming from a Show::update call since the other place reads everything as a short?

After further snooping, I think this is what is going on:

1. In Wizard.java:addShow() (where shows are added), shows get 'descStr' populated but 'descBytes' is set to NULL.
2. Then when wiz.bin is written, writeUTF() is used for the show description since descBytes is NULL but desStr is not.
3. FastRandomFile.java:writeUTF() detects long strings (> 64k) and decides to write 0xFFFF for the 2-byte length, followed by the actual length in 4 bytes. If they are between 32k and 64k the UTF read/write functions still consider them as unsigned.
4. readUTF() recognizes this "auto sizing" and uses an int but the read function in Show.Java does not. It blindly assumes only 2-byte (short) sizes. This means for sizes > 32k they will be negative and for >64k, -1 specfically.

The funny thing is the Show description is written out as a UTF string so other than legacy reasons I cannot see why there even is a byte array version (descBytes). It seems like using readUTF() would fix the issue.

Maybe Jeff can comment on if this is needed or legacy leftovers.

[sdsean]

Well actually I will do that after my next round of testing. . . b/c I'm more worried about it coming from Schedules direct and not the IMBD/CMT stuff.

[sdsean]

Yeah i was doing the same digging as well and was confused as too why there was both a byte array and a string.

That said I currently deleted properities files bigger than 128 KB and am forcing sage to rebuild the db each time trimming down what is happening. It doesnt appear that shows themselves are the problem but rather as mentioned other library imports with gigantic descriptions ending up the description.

So. . .that means that CMT / IMDB is the culprit here. . . (though granted I have been running for a looong time with a lot of stuff stored so it's no surprise that i tripped an edge case)

[wnjj]

Yep, and if there really are reviews and/or other user-contributed data in there more time = larger size as the shows age.

[sdsean]

Alright after further trials and tribulations. . . . I disabled CMT since it basically runs at startup whenever there are not entries in the DB to generate properties files and db meta data. From there I also deleted any .properties files (which i had literally dozens of) that were bigger than 64KB (an educated guess since we're mainly talking about descriptions being too big).

Sure enough, not only did that solve issues with a corrupt wiz.bin generation but favorites and what not seem to stick. I don't know yet if that also solves problems with the client being able to connect to the server on the same machine though given the multi-threaded nature. . .an error thrown from wiz.bin generation seems likely that i might cause other issues. ..

Now. . .what to do about said situation. .. as keeping CMT out of the loop seems like a non starter to me in terms of functionality. . .

Honestly it seems like Show.java needs to be updated to allow much more safe reading and writing particularly when it comes to the description field. As @wnjj noted, it's super odd that there are 2 different description fields at all. . .

But fixing that would then allow plugins to work. . .and while I could understand why we might want to make plugins be more strict, this seems like something that should be done in core anyway, since clearly an import of random properties of various very large sizes and ages can cause the whole system to not work. . ..

As noted I already have a branch that will prevent writing through Show.java to use the byteArray and thereby circumvent some edge cases, but i think there's probably still mroe to do . . . would love to hear more. . .in the mean time I have to not turn on CMT for now to avoid making my properties files grow to big (which stinks but i'll deal with it).

[sdsean]

BTW indeed solving the corrupt wiz.bin issue with description meta-data being too large by nuking properties files also solved my issue of the client not being able to be run on the server at the same time. . .

I'd still like someone to comment though on the bigger issue at hand. . .should I try to submit a PR to git to fix writes of a Show description that are too large? Should we try to fix reads as well (and just force it to the string)? While i admit that it seems logical we could have plugin writers behave more gracefully as well it also seems pretty straight forward to me to fix this in the core to at least keep it from causing such a catastrophic crash.

[wnjj]

Originally Posted by sdsean

BTW indeed solving the corrupt wiz.bin issue with description meta-data being too large by nuking properties files also solved my issue of the client not being able to be run on the server at the same time. . .

I'd still like someone to comment though on the bigger issue at hand. . .should I try to submit a PR to git to fix writes of a Show description that are too large? Should we try to fix reads as well (and just force it to the string)? While i admit that it seems logical we could have plugin writers behave more gracefully as well it also seems pretty straight forward to me to fix this in the core to at least keep it from causing such a catastrophic crash.

Yes, the core should either truncate the too-large strings or change to UTF read so they fit. That will have to be Jeff's call on what & why. Expecting all of the outside sources to voluntarily adhere to sizes isn't practical, IMO.

[sdsean]

PR submitted, we are discussing it there: https://github.com/google/sagetv/pull/386

[wnjj]

Originally Posted by sdsean

PR submitted, we are discussing it there: https://github.com/google/sagetv/pull/386

Very good. It turns out those byte arrays were kind of legacy. Anyway, to fix the read, just change the Show input function from:

    // We lazily create these String to speed up loading time and reduce memory overhead
    int size = in.readShort();
    if (size == 0)
      episodeNameStr = "";
    else {
      episodeNameBytes = new byte[size];
      in.readFully(episodeNameBytes);
    }
    size = in.readShort();
    if (size == 0)
      descStr = "";
    else {
      descBytes = new byte[size];
      in.readFully(descBytes);
    }

to

    episodeNameStr = in.readUTF();
    descStr = in.readUTF();

This works because the readUTF acts like readShort/readFully (but unsigned) and will automatically use an int if size > 64k.

Then in the Show output function, get rid of all of the if/else's and just leave behind:

      out.writeUTF(episodeNameStr);
      out.writeUTF(descStr);

Also, getEpisodeName() and getDesc() can be simplified down to just the last line that returns the string. If you want to delete the '*Desc' variables, be sure to remove the lines in Wizard.java that set them to NULL and maybe search for any other references. I believe Show.java and Wizard.java are it.

[sdsean]

I kept the "deprecated" variables to make it easy for now i case there's some other unforseen consequences.