long transaction causes assertion failure with register format

[taviso]

I was trying out this example John posted to the mailing list:

ledger -f foo.dat reg assets liabilities --daily -n -F '%(date) %12(total)\n'

But it causes an assertion failure for me:

Error: Assertion failed in "/build/ledger-SR1_DQ/ledger-3.1.3/src/unistring.h", line 72:ledger::unistring::unistring(const string&): len < 1024                               

I tracked it down to this transaction, which is a stock purchase followed by a bunch of automatic dividend reinvestments (numbers changed for privacy):

2023/01/01 Opening Balances                                                                                                                                                   
    Assets:Investments:Brokerage  10 ABC @ $1                                                                                                                                 
    Assets:Investments:Brokerage  20 DEF @ $150.10                                                                                                                            
    Assets:Investments:Brokerage  20 DEF @ $155.10                                                                                                                            
    Assets:Investments:Brokerage  20 DEF @ $160.10                                                                                                                            
    Assets:Investments:Brokerage  0.555 DEF @ $139.86                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $105.38                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $106.03                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $100.43                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $110.43                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $167.84                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $154.28                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $137.87                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $167.62                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $103.49                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $183.88                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $164.78                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $124.15                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $189.91                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $165.80                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $172.87                                                                                                                         
    Assets:Investments:Brokerage  0.555 DEF @ $152.30
    Assets:Investments:Brokerage  0.555 DEF @ $175.06
    Assets:Investments:Brokerage  0.555 DEF @ $140.07
    Assets:Investments:Brokerage  0.555 DEF @ $321.53
    Assets:Investments:Brokerage  0.555 DEF @ $173.77
    Assets:Investments:Brokerage  0.555 DEF @ $183.37
    Assets:Investments:Brokerage  0.555 DEF @ $159.54
    Assets:Investments:Brokerage  0.555 DEF @ $107.77
    Assets:Investments:Brokerage  0.555 DEF @ $122.80
    Assets:Investments:Brokerage  0.555 DEF @ $139.38
    Assets:Investments:Brokerage  0.555 DEF @ $134.81
    Assets:Investments:Brokerage  0.555 DEF @ $177.23
    Equity:Opening Balances

Reproduce:

$ ledger --file finance.dat reg -F '%12(total)'
Error: Assertion failed in "/build/ledger-SR1_DQ/ledger-3.1.3/src/unistring.h", line 72:ledger::unistring::unistring(const string&): len < 1024

[taviso]

I guess the issue is just this:

ledger/src/unistring.h

Line 72 in ccbd933

assert(len < 1024);

I don't see any reason why the length has to be less than 1024, and if I just remove that it seems to work.

[taviso]

Is there any possibility this oneline patch could could get applied?

[tbm]

The question is whether there is any buffer that is 1024? If it works when you remove it, maybe the buffer size was updated (I know we updated some buffer sizes) in which case this assert should also be updated, not removed.

But I'm just guessing blindly here...

[taviso]

I guess, but it definitely isn't the correct assertion today and it's effectively causing a crash for me!

If this is going to be a hardlimit, I don't mind - I guess I just need to rewrite some long transactions so they continue working in new versions (I just patch my build to remove this at the moment, but would prefer to get automatic distro updates).

[tbm]

@jwiegley since you're active, can you check which buffer this relates to and what the current acceptable size is.

[jwiegley]

This seems very arbitrary to me, and should be at least as large as the longest line allowed by Ledger.

[jwiegley]

I've submitted a PR that synchronizes this number with MAX_LINE from src/context.h.

[tbm]

Just to refer to the commit:

commit d77e083
Author: John Wiegley johnw@newartisans.com
Date: Tue Mar 7 16:35:51 2023 -0800

Increase string size limit in src/unistring.h assert

[taviso]

FWIW I was able to hit the new limit too, and again just removing the assertion fixes it just fine :(

I really think we should just remove that assertion - isn't it better to maybe have a crash somewhere that nobody is really sure exists, than definitely have a crash somewhere? (an assertion failure is a crash, It breaks my usage).

[jwiegley]

Well, a buffer overrun can be more damaging than just a crash. Maybe we should just crank the buffer size up to ludicrous mode?

[taviso]

I removed the assertion and logged the longest len value I saw, I got 9976. No crash. I recompiled with -fsanitize=address to make sure there isn't any silent memory corruption happening - nothing.

Do you know where the buffer overrun might happen, or what command?

I can try artificially making a ledger that pushes the length even higher, my suspicion is the assertion is just obsolete!

[taviso]

Okay, I just made a transaction with thousands of bogus lots to artificially increase length.

I verified that no length up to 36267 crashes (also with address sanitizer, of course). The output seems correct too.

[taviso]

(It doesn't crash higher than that too, if there is a length where it crashes I don't know what it is)

[jwiegley]

Let's bump to 32K?