With the feature merged (and then only) I'd want to go for version 2.3.0.

Please add marker /* Added in 2.3.0 */ here as well

I noticed that XML_ENTITY_RECURSION_LIMIT is compared with "greater than or equal to" while XML_ENTITY_EXPANSION_SIZE is compared with "greater than". Is that difference intentional?

Code XML_ERROR_ENTITY_EXPANSION_LIMIT seems to be used for two (slightly) different error scenarios now. Is there some security concern why we need to obscure those two cases into one? Else I imagine debugging is going to be more fun if those two cases can be told apart. What do you think?

That looks like we have a single failed test more now? Please share the story with this.

That multiplication may need integer overflow detection/protection. What do you think?

That long should probably be XML_Size instead, especially for people with XML_LARGE_SIZE defined.

To my best knowlegde signed integer overflow is undefined behavior, hence we'd need to prevent overflow before it even happened. Both index and XML_ENTITY_EXPANSION_RATIO are signed as far as I can see.

expat.h doesn't define MAX values for types. That makes it kinda complicated to check for overflows. What's your proposal?

We could try division instead to bypass that issue. On a chalkboard I'd write in order:

1. Right now we have:
   parser->m_entityExpansionLen > limit.
2. Inlined, that's:
   parser->m_entityExpansionLen > index * XML_ENTITY_EXPANSION_RATIO.
3. If we devide both sides by XML_ENTITY_EXPANSION_RATIO we get:
   parser->m_entityExpansionLen / XML_ENTITY_EXPANSION_RATIO > index.
4. If we take the floor effect of integer division into account and correct for it to accept the same range we get:
   (parser->m_entityExpansionLen + (XML_ENTITY_EXPANSION_RATIO - 1)) / XML_ENTITY_EXPANSION_RATIO > index.

Does that look about right?

That could be one way. Another way could be leveraging (XML_Size)(-1) since that type is unsigned. I'd need to think about that more.

It would be awesome to have all the version bump changes split off to a single dedicated commit following the others. Thanks!

Why prefer XML_Index over XML_Size here? Right now we have a length-index mix here. If the idea is to save some casts later, I'd want to be sure that the loss in clean semantics is worth the gain.

It would be cool to avoid that hardcoding of 8 (and 9 a few lines down) one way or another (e.g. format string or doing things on preprocessor level).

The 8 and 9 shouldn't have landed in the PR. They are remnants of a debugging session.

Not a big problem, but huge_entities would read a lot better than he in my eyes (and it's less guessable that nsalloc, for instance). Just my two cents.

I changed he to huge_entities everywhere.

I needed to lookup if expect_failure does parsing inside or not because the name does not say.
If you feel like adding a dedicated commit upfront that renames it to something like parse_and_expect_failure, my vote for it. Just an idea.

Nice to have that math, I tried to verify its details.
I get that 2304 is slightly bigger than 230 x 10 (=default ratio limit) so that extra 4 characters triggers the limit.

The first line is clear:

• 50 seems to be the length of <!DOCTYPE he [<!ELEMENT he (#PCDATA)*><!ENTITY e '.
• Followed by 64 for entity value A...P.
• 8 is length of '>]><he>
• 36 x 3 is all those &e;.

For the second I now notice that 33 expansions of 64 bytes should probably read 36 rather than 33.

Good catch! I forgot to update that line when I added the <!ELEMENT> line.

I'm still thinking: If we don't need to be compatible with defusedexpat names for these constants, it would be nice to get them in good consistent shape. Just to give an example, I'd find something like

• XML_ERROR_ENTITY_VIOLATION_SIZE
• XML_ERROR_ENTITY_VIOLATION_RATIO
• XML_ERROR_ENTITY_VIOLATION_DEPTH

more consistant. Maybe we can find some names nice that have that property. (The current _LIMIT part is also not ideal in my eyes.) What do you think?

Your names sound better than mine.

I'm starting to think if "nesting" would be a better term since "recursion" seems to imply that something can reference itself (which XML entities cannot).

(Just to have said it somewhere: We will need to support adjustment of those defaults at runtime at some point, maybe even before merging to master. I'm all in with getting other things polished first, though.)

No, I wouldn't go there. I made the settings flexible in defusedexpat. In retrospective it was unnecessary API bloat. Even libxml2 doens't allow to tune the limits. It has only an off-switch for limitation. If libxml2 doesn't need extra options, then expat surely doesn't need them at all.

Interesting thought. Let me think about it more.

It would be nice if we could cover those two functions by some simple test(s) as well. Some idea for a test:

1. Call to XML_GetOptions returns default options
2. Call to XML_SetOptions succeeds
3. Call to XML_GetOptions returns what was set by XML_SetOptions just before
4. Parsing is started
5. Call to XML_SetOptions fails (due to parser started)
6. Call to XML_GetOptions still returns unmodified previous options

Could also be split in half to make two tests where the second is dedicated to changes at runtime.

Good idea! I added a new test scenario as you proposed.

Maybe append " during/while parsing"

Maybe append " (i.e. previously set options)"

That dereference makes the usage of XML_SetOption overly complicated. Compare current:

XML_Bool ENABLED = XML_TRUE;
XML_SetOption(parser, XML_OPTION_HUGE_ENTITIES, &ENABLED);

with possible

XML_SetOption(parser, XML_OPTION_HUGE_ENTITIES, (void *)XML_TRUE);

I believe each option should have a say if it needs a dereference or not and save that extra indirection for plain integer ones. What do you think?
I'm also wondering about use of uintptr_t/intptr_t rather than void *, just an idea.

My vote for mentioning "billion laughs" for an example and "DoS" to make things less abstract to people new to these topics.

For clearer semantics, I'd consider turning this into:

const XML_Index minimumRequiredIndex = (XML_Index)
  ((parser->m_entityExpansionLen + (XML_ENTITY_EXPANSION_RATIO - 1))
    / XML_ENTITY_EXPANSION_RATIO);
if (index < minimumRequiredIndex) {

Please note the flip of comparision order and operator.

Regarding the cast: The largest positive value for XML_Index and XML_Size are factor 2 apart, so if we divide by XML_ENTITY_EXPANSION_RATIO and that number is greater or equal 2, even m_entityExpansionLen of (XML_Size)(-1) would fit in XML_Index minimumRequiredIndex without truncation.

I believe this is CVE-2013-0340. If so, let's mention it.

Let's also mention DoS and security here in case this ends up being the place where peokle look at the most.

Since options is plural, let's make it support multiple options. How about:

file="$1"
shift
reldir="$1"
shift
options=( "$@" )
$XMLWF -p -N -d "$OUTPUT$reldir" "${options[@]}" "$file" > outfile || return $?

Would that work?

Why that guard? Can that file ever be missing?

If Failed count goes up by one for tests/xmlconf/xmltest/valid/ext-sa/012.xml without -H why does Passed count not go up by one for the new additional test with -H?

• Now that we also have a hash table option in here, maybe the entity ones should include the word "entity". On a blank slate, I'd maybe write:
  • XML_OPTION_ENTITIES_HUGE
  • XML_OPTION_ENTITIES_MAX_DEPTH
  • XML_OPTION_ENTITIES_MAX_RATIO
  • XML_OPTION_ENTITIES_MAX_SIZE
  • XML_OPTION_HASH_TABLE_DTD_MAX_ENTRY_COUNT
• For size, we should mention XML_Chars to make clear it's not bytes. Maybe even append _CHARS to that name?
• After = 1 I see no = 2, = 4 and so on. That looks unintended
• Why go for int rather than unsigned int?

XML_OPTION_ENTITIES_MAX_DEPTH is misleading. It's not a limitation of the depths but how many expansions are nested. For example

<!DOCTYPE xmlbomb [
<!ENTITY a "1234567890" >
<!ENTITY b "&a;&a;&a;&a;&a;&a;&a;&a;&a;&a;">
<!ENTITY c "&b;&b;&b;&b;&b;&b;&b;&b;&b;&b;">
]>
<bomb>&c;</bomb>

has depths of 3 but nesting of 111 (1c + 10b + 1010c).

Maybe depth and nesting are both mis-leading. Maybe path, steps, calls, ..? In a way it's an expansion tree and we limit regarding the number of total nodes of that tree then.

That would be 2MB with -DXML_UNICODE (or even 4MB if we get UTF32 support on day) since we check for number of XML_Chars.

Good catch, I'll update the comment.

XML_SetOptions() should probably be XML_SetOption without s and without braces for consistency. There are more (leftover) occurrences like that below.

Maybe XML_ERROR_HASH_TABLE_VIOLATION_SIZE with _SIZE appended so that it's clear what the violation is about?

XML_ERROR_HASH_TABLE_SIZE_VIOLATION ?

Would sound good on its own but not match the *_VIOLATION_* schema above. As you prefer.

Billion laughs demos use a nesting level of 10. Our default needs to be below 10 then.

The settings limits the amount of nested expansions, not the maximum depths of an expansion stack. Perhaps we need a better name or explanation?

Yes, we need a better name then. Some ideas mentioned further up.

Most of these cases are not covered by test yet, it seems.

That XML_L seems to be off the indentation grid.

I think there's a > in parser-m_limit missing here.

We may also need something like limitReset to be used in XML_ParserReset.

Good catch! I'll add it to XML_ParserReset.

Is limitDestroy called at all needed places, yet? I expected a call from XML_ParserFree.

It's used in two places. I also need to free the structure in case another allocation in parserCreate fails. But it's just a simple FREE. I'll get rid of the function.

Typo XML_Cha)

Let's use XML_Size or XML_Index for entitiesMaxRatioThreshold.