search

coelckers / prboom-plus

This is a cleaned up copy of the PrBoom+ SVN repository as a courtesy for those interested in forking that port
289 stars 115 forks source link

Maps >100 crash #568

Closed JadingTsunami closed 1 year ago

JadingTsunami commented 1 year ago

The UMAPINFO spec gives "MAP100" as an example, but defining maps >99 will crash both PrBoom+ and dsda-doom.

WAD entry limits are 8 characters. Map markers consume either 2 (ExMy format) or 3 (MAPxx format) characters from that total. GL nodes prefix the map marker with GL_ which drops another 3 characters.

Options:

  1. Cap MAPxx at 99 and ExMy at len(x)+len(y) <= 3
  2. Skip GL nodes in maps that don't fit the criteria from (1) and cap MAPxx at 99999 and ExMy at len(x)+len(y) <= 6
  3. Use the limits from (2) for UDMF and limits from (1) for "legacy" map types.

Posting here for discussion. Fixing the crash is trivial but has implications for the spec.

fraansg commented 1 year ago

Fixing the crash is trivial but has implications for the spec.

In what way would it be?

I don't think fixing it has implications for the spec as such, if at the end of the day the spec is just a standard, and each port has to deal with finding a way to comply with said standard

JadingTsunami commented 1 year ago

I don't think fixing it has implications for the spec as such, if at the end of the day the spec is just a standard, and each port has to deal with finding a way to comply with said standard

The UMAPINFO spec will need to be updated to specify the valid limits.

fraansg commented 1 year ago

The UMAPINFO spec will need to be updated to specify the valid limits.

I think it is better to keep a minimum approach on the content of the spec itself, even more if this limits are not inherent to it. Anyway, it's just my opinion

JadingTsunami commented 1 year ago

I think it is better to keep a minimum approach on the content of the spec itself, even more if this limits are not inherent to it.

In general it is true that for specs that define abstractions it's best to leave out hard limits. In this case the limits are indeed inherent to the underlying format. As a cross-port standard this doesn't make much sense to me. But others may have thoughts as well.

kraflab commented 1 year ago

This basically means prboom+ is not fully umapinfo-compliant. I think option 2 is the best choice. There's still this implicit limit caused by lump name constraints, but that doesn't need to be in the spec itself, since it's format-dependent.

I haven't touched this topic because there are a lot of things to consider for a comprehensive solution. What do we do if a umapinfo lump defines MAP001? P_SetupLevel will fail because it will look for MAP01. I'm going to implement map name handling for generic map lumps in dsda-doom at some point but that's probably overboard in prboom+.

I do wonder if this was an oversight in the spec or just not tested, since prboom+ has the reference implementation but was never compliant.

JadingTsunami commented 1 year ago

I think option 2 is the best choice.

I'm OK with this one. For Pr+ we can just use an 8-character limit when looking for related lumps. The failure, if any, can explain the rest.

On the "001" specifier, feels like a spec update as the spec should at a minimum explain the data format.