[all versions] off by one error in sds string lib flb_sds_printf triggered by specific conditions

[PettitWesley]

Discovered here in a conversation on the linked bug fix: #7129 (comment)

flb_sds_printf will re-alloc if needed: https://github.com/fluent/fluent-bit/blob/master/src/flb_sds.c#L437

However, note the inclusive and exclusive wording in the docs below:

The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated. (See also below under NOTES.)

https://linux.die.net/man/3/vsnprintf

So the condition on re-alloc'ing the string in the code should be >= and the re-alloc should be +1 larger I think.

Demo

branch: https://github.com/PettitWesley/fluent-bit/commits/sds-off-by-1-demo

See the demo here: PettitWesley@57925b8

And fix here: PettitWesley@a333eeb

unit test here: PettitWesley@8075d6e

[ec2-user@ip-10-192-11-106 build]$ ./bin/fluent-bit -i dummy -o stdout
Fluent Bit v2.1.0
* Copyright (C) 2015-2022 The Fluent Bit Authors
* Fluent Bit is a CNCF sub-project under the umbrella of Fluentd
* https://fluentbit.io

[2023/04/08 00:06:19] [ info] [fluent bit] version=2.1.0, commit=06ae7f1756, pid=11668
[2023/04/08 00:06:19] [ info] [storage] ver=1.4.0, type=memory, sync=normal, checksum=off, max_chunks_up=128
[2023/04/08 00:06:19] [ info] [cmetrics] version=0.6.1
[2023/04/08 00:06:19] [ info] [ctraces ] version=0.3.0
[2023/04/08 00:06:19] [ info] [input:dummy:dummy.0] initializing
[2023/04/08 00:06:19] [ info] [input:dummy:dummy.0] storage_strategy='memory' (memory only)
[2023/04/08 00:06:19] [ info] [sp] stream processor started
[2023/04/08 00:06:19] [ info] [output:stdout:stdout.0] worker #0 started
[2023/04/08 00:06:21] [ info] Example below will work as expected, note that all letters up to z are printed:
[2023/04/08 00:06:21] [ info] [sds] len=64
[2023/04/08 00:06:21] [ info] [sds] size1=66
[2023/04/08 00:06:21] [ info] [sds] size2=66
[2023/04/08 00:06:21] [ info] [sds] head->len=66
[2023/04/08 00:06:21] [ info] TEST: A0123456789 this-is-54-chars-1234567890-abcdefghijklmnopqrstuvwxyz
[2023/04/08 00:06:21] [ info] Example below shows bug, note that all letters up to z are not printed:
[2023/04/08 00:06:21] [ info] [sds] len=64
[2023/04/08 00:06:21] [ info] [sds] size1=64
[0] dummy.0: [[1680912380.264105949, {}], {"message"=>"dummy"}]
[2023/04/08 00:06:21] [ info] [sds] size2=64
[2023/04/08 00:06:21] [ info] [sds] head->len=64
[2023/04/08 00:06:21] [ info] TEST: 123456789 this-is-54-chars-1234567890-abcdefghijklmnopqrstuvwxy

Conditions that cause it

See the len logic here: https://github.com/fluent/fluent-bit/blob/master/src/flb_sds.c#L411

I think to cause this bug to surface, the variables used in the format must be longer than the format in just the right way to get vsnprintf returned size that's equal to the currently alloc'd size.

This is what I used in the demo above:

    flb_sds_t tmp;
    flb_sds_t test = flb_sds_create_size(64);
    if (!test) {
        flb_errno();
        FLB_OUTPUT_RETURN(FLB_ERROR);
    }

    flb_info("Example below will work as expected, note that all letters up to z are printed:");

    tmp = flb_sds_printf(&test, "A0123456789 %s", "this-is-54-chars-1234567890-abcdefghijklmnopqrstuvwxyz");
    flb_info("TEST: %s", tmp);

    flb_sds_t test2 = flb_sds_create_size(64);
    if (!test2) {
        flb_errno();
        FLB_OUTPUT_RETURN(FLB_ERROR);
    }

    flb_info("Example below shows bug, note that all letters up to z are not printed: ");
    tmp = flb_sds_printf(&test2, "123456789 %s", "this-is-54-chars-1234567890-abcdefghijklmnopqrstuvwxyz");
    flb_info("TEST: %s", tmp);

[PettitWesley]

Will submit fix PR from above commit very soon.

[PettitWesley]

See relevant similar code in upstream/original SDS library: https://github.com/antirez/sds/blob/master/sds.c#L559

Looks like that logic recently changed from a much different older code: antirez/sds@a9a03bb

https://github.com/antirez/sds/blame/fb463145c9c245636feb28b5aac0fc897e16f67e/sds.c#L543

[nokute78]

Note: #7042 and #7041

[leonardo-albertovich]

Thanks for the links @nokute78, I was sure I had verified this issue with someone else in the past and thought it would've definitely been fixed but I guess it never took off.

It's a legit bug the patches are wrong because flb_sds_increase increments the buffer size by N, doesn't resize the buffer to N which means 438 should be :

tmp = flb_sds_increase(s, flb_sds_alloc(s) - size);

Which in the case of the 64 character string would mean passing 0 as the increment size to the function but considering that the function adds 1 character for the terminator it works.

If the patch is applied as is the buffer would go from 80 characters to 145 whereas with the proper change it would go from 80 to 81.

[PettitWesley]

@leonardo-albertovich flb_sds_increase is used twice in the function, right now the first usage increases the alloc by 64... should I also change that to increase it to 64??

[leonardo-albertovich]

Sure.

[PettitWesley]

@leonardo-albertovich sorry actually I think it should be:

tmp = flb_sds_increase(s, size - flb_sds_avail(s) + 1);

We check the avail in the condition which makes sense since the some of the alloc might already be taken by an existing string.

[PettitWesley]

I will add unit test for this bug as well

[PettitWesley]

The test here repros it: PettitWesley@8075d6e

[leonardo-albertovich]

You are right, I just approved your PRs, thanks a lot!

[github-actions]

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 5 days. Maintainers can add the exempt-stale label.

[github-actions]

This issue was closed because it has been stalled for 5 days with no activity.

[PettitWesley]

This was fixed:
#7148
#7147
#7146