bug/limit with big queue

[magicdoublem]

Looks like I've hit a wall here. NZBget crashs after loading a heavy queue (btw, those tiny files can slow down the starting process when the state file is being rebuild).

This is what windbg puts out:

(4b3c.422c): Security check failure or stack buffer overrun - code c0000409 (!!! second chance !!!)
Subcode: 0x5 FAST_FAIL_INVALID_ARG
nzbget!iconv_canonicalize+0xfd31d2:
00007ff6`6fe5f65c cd29 int 29h

The log doesnt put out anything interesting, a dmp file hasnt been crated, even though it was enabled and i'm using the debug build.

[dnzbk]

Approximately how large is the queue?

[magicdoublem]

We are talking about something around 51k entries.

[magicdoublem]

The newest build (all those fixes are very much appreciated btw) brought up a more interesting error-message I guess:

Wed Apr 30 17:14:42 2025 37064 27504 DEBUG Saving nzb list to disk (DiskState.cpp:464:DiskState::SaveQueue)
Wed Apr 30 17:14:43 2025 37064 27504 ERROR Unhandled Exception: code: 0xC0000005, flags: 0, address: 0xACE6F348
Wed Apr 30 17:14:43 2025 37064 27504 ERROR Stacktrace: (0) :

That last line repeats a few times...

A while back ended up using NZBget's queue as lazy "bookmark-system" for on-demand-usage, and when I broke nzb360s implementation I wanted to see if NZBget can break too.

[dnzbk]

Thanks, it will be useful. At least now we have the exact place in the code.
I’ll investigate that.

[magicdoublem]

A bit more digging on this... The issue might actually go a bit deeper than thought. In some cases nzbget did start and *arrs added some new stuff, then it crashs (some time later windows crashed occasionally, but at that point I didn't connect these two issues). Now, funny or not, I'm at a point where windows crashs reproducable with nzbget causing it. The minidump shows something like hits:

`DRIVER_OVERRAN_STACK_BUFFER (f7)
A driver has overrun a stack-based buffer. This overrun could potentially
allow a malicious user to gain control of this machine.
DESCRIPTION
A driver overran a stack-based buffer (or local variable) in a way that would
have overwritten the function's return address and jumped back to an arbitrary
address when the function returned. This is the classic "buffer overrun"
hacking attack and the system has been brought down to prevent a malicious user
from gaining complete control of it.
Do a kb to get a stack backtrace -- the last routine on the stack before the
buffer overrun handlers and BugCheck call is the one that overran its local
variable(s).
Arguments:
Arg1: ffff9c2abe8c2038, Actual security check cookie from the stack
Arg2: 000098cb21269ae1, Expected security check cookie
Arg3: ffff6734ded9651e, Complement of the expected security check cookie
Arg4: 0000000000000000, zero

Debugging Details:

KEY_VALUES_STRING: 1

Key  : Analysis.CPU.mSec
Value: 3125

Key  : Analysis.Elapsed.mSec
Value: 4385

Key  : Analysis.IO.Other.Mb
Value: 0

Key  : Analysis.IO.Read.Mb
Value: 1

Key  : Analysis.IO.Write.Mb
Value: 0

Key  : Analysis.Init.CPU.mSec
Value: 296

Key  : Analysis.Init.Elapsed.mSec
Value: 26684

Key  : Analysis.Memory.CommitPeak.Mb
Value: 90

Key  : Analysis.Version.DbgEng
Value: 10.0.27793.1000

Key  : Analysis.Version.Description
Value: 10.2410.02.02 amd64fre

Key  : Analysis.Version.Ext
Value: 1.2410.2.2

Key  : Bugcheck.Code.LegacyAPI
Value: 0xf7

Key  : Bugcheck.Code.TargetModel
Value: 0xf7

Key  : Dump.Attributes.AsUlong
Value: 0x21808

Key  : Dump.Attributes.DiagDataWrittenToHeader
Value: 1

Key  : Dump.Attributes.ErrorCode
Value: 0x0

Key  : Dump.Attributes.KernelGeneratedTriageDump
Value: 1

Key  : Dump.Attributes.LastLine
Value: Dump completed successfully.

Key  : Dump.Attributes.ProgressPercentage
Value: 0

Key  : Failure.Bucket
Value: 0xF7_MISSING_GSFRAME_nt!_report_gsfailure

Key  : Failure.Hash
Value: {82d2c1b5-b0cb-60a5-9a5d-78c8c4284f84}

Key  : Hypervisor.Enlightenments.ValueHex
Value: 0x7417df84

Key  : Hypervisor.Flags.AnyHypervisorPresent
Value: 1

Key  : Hypervisor.Flags.ApicEnlightened
Value: 0

Key  : Hypervisor.Flags.ApicVirtualizationAvailable
Value: 1

Key  : Hypervisor.Flags.AsyncMemoryHint
Value: 0

Key  : Hypervisor.Flags.CoreSchedulerRequested
Value: 0

Key  : Hypervisor.Flags.CpuManager
Value: 1

Key  : Hypervisor.Flags.DeprecateAutoEoi
Value: 1

Key  : Hypervisor.Flags.DynamicCpuDisabled
Value: 1

Key  : Hypervisor.Flags.Epf
Value: 0

Key  : Hypervisor.Flags.ExtendedProcessorMasks
Value: 1

Key  : Hypervisor.Flags.HardwareMbecAvailable
Value: 1

Key  : Hypervisor.Flags.MaxBankNumber
Value: 0

Key  : Hypervisor.Flags.MemoryZeroingControl
Value: 0

Key  : Hypervisor.Flags.NoExtendedRangeFlush
Value: 0

Key  : Hypervisor.Flags.NoNonArchCoreSharing
Value: 1

Key  : Hypervisor.Flags.Phase0InitDone
Value: 1

Key  : Hypervisor.Flags.PowerSchedulerQos
Value: 0

Key  : Hypervisor.Flags.RootScheduler
Value: 0

Key  : Hypervisor.Flags.SynicAvailable
Value: 1

Key  : Hypervisor.Flags.UseQpcBias
Value: 0

Key  : Hypervisor.Flags.Value
Value: 55185662

Key  : Hypervisor.Flags.ValueHex
Value: 0x34a10fe

Key  : Hypervisor.Flags.VpAssistPage
Value: 1

Key  : Hypervisor.Flags.VsmAvailable
Value: 1

Key  : Hypervisor.RootFlags.AccessStats
Value: 1

Key  : Hypervisor.RootFlags.CrashdumpEnlightened
Value: 1

Key  : Hypervisor.RootFlags.CreateVirtualProcessor
Value: 1

Key  : Hypervisor.RootFlags.DisableHyperthreading
Value: 0

Key  : Hypervisor.RootFlags.HostTimelineSync
Value: 1

Key  : Hypervisor.RootFlags.HypervisorDebuggingEnabled
Value: 0

Key  : Hypervisor.RootFlags.IsHyperV
Value: 1

Key  : Hypervisor.RootFlags.LivedumpEnlightened
Value: 1

Key  : Hypervisor.RootFlags.MapDeviceInterrupt
Value: 1

Key  : Hypervisor.RootFlags.MceEnlightened
Value: 1

Key  : Hypervisor.RootFlags.Nested
Value: 0

Key  : Hypervisor.RootFlags.StartLogicalProcessor
Value: 1

Key  : Hypervisor.RootFlags.Value
Value: 1015

Key  : Hypervisor.RootFlags.ValueHex
Value: 0x3f7

BUGCHECK_CODE: f7

BUGCHECK_P1: ffff9c2abe8c2038

BUGCHECK_P2: 98cb21269ae1

BUGCHECK_P3: ffff6734ded9651e

BUGCHECK_P4: 0

FILE_IN_CAB: 050225-18468-01.dmp

TAG_NOT_DEFINED_202b: *** Unknown TAG in analysis list 202b

DUMP_FILE_ATTRIBUTES: 0x21808
Kernel Generated Triage Dump

FAULTING_THREAD: ffffd081748cf080

SECURITY_COOKIE: Expected 000098cb21269ae1 found ffff9c2abe8c2038

BLACKBOXBSD: 1 (!blackboxbsd)

BLACKBOXNTFS: 1 (!blackboxntfs)

BLACKBOXPNP: 1 (!blackboxpnp)

BLACKBOXWINLOGON: 1

CUSTOMER_CRASH_COUNT: 1

PROCESS_NAME: nzbget.exe

STACK_TEXT:
ffff9c878063f818 fffff8059c73fd25 : 00000000000000f7 ffff9c2abe8c2038 000098cb21269ae1 ffff6734ded9651e : nt!KeBugCheckEx
ffff9c878063f820 fffff8059ca4c0a7 : 0000000000000001 0000000000000001 ffff9c878063f888 0000000000000000 : nt!_report_gsfailure+0x25
ffff9c878063f860 fffff8059ca4a809 : ffffd081748cf080 0000000000000000 ffff9c878063faa0 000000ad3eefd8d0 : nt!ObCloseHandleTableEntry+0x3e7
ffff9c878063f9b0 fffff8059c8b8655 : 000001105b72d700 0000000000000494 ffffd081748cf080 000000ad3eefdb90 : nt!NtClose+0xe9
ffff9c878063fa20 0000000000000000 : 0000000000000000 0000000000000000 0000000000000000 0000000000000000 : nt!KiSystemServiceCopyEnd+0x25

SYMBOL_NAME: nt!_report_gsfailure+25

MODULE_NAME: nt

IMAGE_NAME: ntkrnlmp.exe

IMAGE_VERSION: 10.0.26100.3912

STACK_COMMAND: .process /r /p 0xffffd08fa4bd5080; .thread 0xffffd081748cf080 ; kb

BUCKET_ID_FUNC_OFFSET: 25

FAILURE_BUCKET_ID: 0xF7_MISSING_GSFRAME_nt!_report_gsfailure

OSPLATFORM_TYPE: x64

OSNAME: Windows 10

FAILURE_ID_HASH: {82d2c1b5-b0cb-60a5-9a5d-78c8c4284f84}

Followup: MachineOwner
---------`

So, here are some thoughts. Please note, my coding-days are far behind, but I'm trying to get back at it. One thing that pops up in my thoughts is the size of that queue directory, especially the amount of state-files. It has grown to 2.671.827 files (no joke). That number alone already breaks (signed) int32. So if there is a counter or something in nzbget...

It also puts the windows limit of files per directory (which is defined by an unsigned int32 it seems) within reach. While totally not the usual queue size, I'd say it's somewhat easy to reach if you let some *arrs go crazy and tend to use the queue as bookmark for nzbs. Just noting, that there is a technical limitation here and things can go even wilder than just having nzbget crash as it seems.

Besides, the current queue concept shows some flaws when it grows. Such an amount of files, flat-saved within a single directory can become problematic to handle. It takes a while to open that directory alone, even from a fast ssd. Fun stuff to witness. It also wastes a lot of space in such cases, and nzbget takes ages to start up if it thinks it has to rebuild some files. A few ideas of different ideas do pop up, but changing how the queue is saved will of course break backward-compatibility, and my skills aren't there yet anyway.

[dnzbk]

Thanks for the investigation

It has grown to 2.671.827 files (no joke). That number alone already breaks (signed) int32. So if there is a counter or something in nzbget...

This is exactly what came to mind. The problem lies in the line:
outfile.PrintLine("%i", (int)queue->size());
from
DiskState.cpp:466:DiskState::SaveQueue
and then we do:

int size;
if (infile.ScanLine("%i", &size) != 1) goto error;

DiskState.cpp:473:DiskState::LoadQueue

size() returns uint32 or uint64 depending on the architecture. The value 2,671,827 exceeds INT_MAX, leading to an integer overflow when casting to int. This signed integer overflow in results in undefined behavior, which can easily cause the application to crash.

...Besides, the current queue concept shows some flaws when it grows. Such an amount of files, flat-saved within a single directory can become problematic to handle. It takes a while to open that directory alone, even from a fast ssd. Fun stuff to witness. It also wastes a lot of space in such cases, and nzbget takes ages to start up if it thinks it has to rebuild some files. A few ideas of different ideas do pop up, but changing how the queue is saved will of course break backward-compatibility, and my skills aren't there yet anyway.

I completely agree. Addressing this efficiently requires a significant architectural change. I suppose, switching to a database like SQLite could solve the performance issues with large queues and huge amount of files, but it would indeed involve a lot of code refactorings.

[magicdoublem]

Yes, agreed, sqlite would probably be the most elegant and future-proof solution. Putting these files into sub-dirs, just stupidly using their first digit (or a few) as directory-name, could be a doable quick and dirty fix that effectively eases the situation significantly.

On the weekend I managed to get the dev-env running and nzbget compiling (thanks to the nice documentation here). I made some first attempts. 466 and 473 had my attention for a moment, but things didn't change and the frustration was high.

"size()" depending on architecture was "THE" hint I needed, since I first tried to use uint32, but compiled for 64bit. Now the outcome makes sense. Thanks man! :)

With uint64 I'm out of crashland, but It's not enough to make things work again. So I found a few more pieces that needed to be touched, LoadProgress and some loops.

Now I'm halfway there I guess, it starts now, loads the queue, keeps running, but there is still something going wrong with attaching all the infos to a job. ...more hunting to do.

[magicdoublem]

I could use a little idea/hint, where to look. With all my experimental upgrades of "a few" variables to 64bit i got the daemon running and loading/saving the queue just fine, running for hours.... BUT! as soon as I open the webui it crashes within a minute, bevore that, the WebUI works fine.

I still don't have figured out/setup a usable way to actually debug whats going on there and what stuff gets called.

Do you have an idea where I should look?

[dnzbk]

Since the webui causes the crash, I’d focus on XmlRpc.cpp It handles communication with the webui.
Check the methods that send queue data. Maybe your 64-bit changes caused a problem with how the webui reads/saves the data.
Sorry, I haven’t had time to look into it myself.
Hope that helps.

[magicdoublem]

Yup, that was a good hint. Things are working now it seems. But of course I have to do some stress-testing before being sure, and maybe I'll take back some changes here and there... Though, going bigger didn't hurt performance and memory-consumption and even might prevent overflows in the future, who knows?

[magicdoublem]

I'm making progress, but don't know if this is a direction that would be considered committable.

After a lot of experimenting, back and forth, and a few moments of reconsiderations, I tend to move as much int to int64 as possible. There are those places where it's necessary to handle a big queue and lots of files. This did lead to NextParamAsInt, which I needed to output int64, so of course everything using this routine needed to move to int64 too, as I don't want to overcomplicate that routine or put in variants.

This ended up being a "fun" rabbit-hole.

In the end I don't think it hurts to upgrade int to int64, memory consumption didn't change in a noticable way. Having more headroom might also prevent from overflows here and there, we are just a malformed nzb-file or someone with a huge queue ;) away from that, how knows what else.

Also, using just one type of int makes code easier to service in the future, which is one reason why I did throw away the idea of using uint32 here and there for the time being. Moving from int32 to int64 is the easiest and safest way I think, even if some stuff will never really need an int64 (as some of it never needed an int32 either).

I also wanted to get rid of atoi/atoll and added a little replacement-routine using boost/spirt/qi, which noticeably speeds things up and should be more safe. This is something I just started testing.

[dnzbk]

This ended up being a "fun" rabbit-hole.

This is exactly what I expected for. That’s why I don’t want to hurry with it. Your investigation is helpful, but we need to carefully weigh the advantages against the potential complications, particularly given our need to support a wide range of platforms and architectures. Changes like this can have unexpected consequences across different systems, so we need to be thorough.

I also wanted to get rid of atoi/atoll and added a little replacement-routine using boost/spirt/qi, which noticeably speeds things up and should be more safe. This is something I just started testing.

boost.spirit is indeed a good choice. However, adding a new dependency into a project with our diverse platform and architecture support always presents a significant challenge.
BTW, I've improved the efficiency and safety of Util::StrToNum so you can use it.

I’ll keep an eye on your fork and address the issue as soon as possible, considering your changes.

[magicdoublem]

I completely agree, no need to hurry here. This needs extensive testing and consideration. My stuff is meant to be experimental research at best. I had lots of unexpected behavior in the past weeks, so I know what you're saying. ;) Unfortunately lots of things rely on each other here, so doing things like that step by step will be difficult, I tried to create a minimal patch, but was back in crashland real quick.

Boost, yeah, agreed, would need extensive testing and the idea of always having a fallback for boosted-functions would be something I'd consider.

Thanks a lot for the improved function!!!

[magicdoublem]

Just a side-note, I did replace almost all atoi/atoll-calls with StrToNum, which works flawlessly so far. There are a few left that need more touching, and .value_or(0) could probably replaced with something smarter here and there, but one major gain is, the app doesn't crash windows anymore, when I get it to crash. ;)

My current build runs smoothly, with heavy testing.

But I'd like to take back int64 on a few places. For that, could we modify NextParamAsInt into a template-based function spitting out variants of ints, like StrToNUm? Maybe call it NextParamAsNum and keep the old one until the transition is complete.

[dnzbk]

But I'd like to take back int64 on a few places. For that, could we modify NextParamAsInt into a template-based function spitting out variants of ints, like StrToNUm? Maybe call it NextParamAsNum and keep the old one until the transition is complete.

Of course, it’s technically possible. You could modify the method’s declaration to directly accept an int64_t* or create a function overload:

bool XmlCommand::NextParamAsInt(int64_t* value);

And you could use StrToNum within the function where needed.
However, it’s important to note that XML-RPC specification doesn’t natively support int64 as a data type. This contrasts with JSON-RPC, which does support int64.
For XML-RPC, we would need to continue handling 64-bit values by processing the Hi and Lo fields (e.g., FileSizeHi and FileSizeLo) and combining them to construct the full int64 value.

[magicdoublem]

And you could use StrToNum within the function where needed. However, it’s important to note that XML-RPC specification doesn’t natively support int64 as a data type. This contrasts with JSON-RPC, which does support int64. For XML-RPC, we would need to continue handling 64-bit values by processing the Hi and Lo fields (e.g., FileSizeHi and FileSizeLo) and combining them to construct the full int64 value.

Thanks again for your thoughts. That last thought made me rethink and reconsider once again, what I would aim for as a committable fix, even though my current build runs smoothly with an uptime of about a week. I'll test a few more ideas soonish.