Jeff Bindel:
> Wietse,
>
> Thank you for the review (list reply and the Sep 16 follow-up). You are
> right on the substantive points. Short answers, then a proposed revision.
>
> 1) Scope. This tip is ARGV-only. I am not proposing to annotate every
> Postfix sequence type. If ARGV is useful and the shape is right, later
> types can be considered one at a time.
>
> 2) ARGV.len meaning / null terminator. Agreed ? my patch was wrong to
> treat len as a count that leaves the trailing NULL outside the
> __counted_by bound, and wrong to retarget ARGV_FAKE*_BEGIN around that.
> Postfix needs the null terminator as a valid array element. I should not
> have changed the meaning of ARGV.len.
>
> 3) Allocation-size / Clang. Postfix already allocates
> (len + 1) * sizeof(char *) and keeps argc < len so argv[argc] can hold
> NULL. That matches the Clang model where __counted_by describes how many
> elements are available. The revised approach I prefer is:
>
> - leave ARGV.len's existing meaning alone;
> - annotate argv with POSTFIX_COUNTED_BY_OR_NULL(len + 1) so the bound
> is the real allocation size and includes the null-terminator slot;
> - revert the ARGV_FAKE*_BEGIN len/argc edits (back to len == argc with
> the stack array still holding the trailing NULL).
>
> If you would rather see a different expression of that bound, I will
> follow your preference.
>
> 4) Unused macros. Agreed ? POSTFIX_SIZED_BY / POSTFIX_SIZED_BY_OR_NULL
> are unused and should be deleted.
>
> 5) Update order / paired assignment. Agreed again. Updating len before
> argv is an anti-pattern relative to normal Postfix practice and to Clang's
> paired-assignment rule
> (
> https://www.google.com/url?q=https://clang.llvm.org/docs/BoundsSafetyImplPlans.html%23paired-assignment-check&source=gmail&ust=1789825604203000&sa=E
> ).
> The claim that "capacity before pointer" was required to avoid a compiler
> error or sanitizer trap was incorrect; that was an AI-assisted mistake on
> my part, not something I verified against Clang. Revised code should
> assign the pointer and then the length (side by side, no intervening
> side effects), consistent with Postfix and with the docs.
>
> I can either withdraw this tip or send a revised format-patch once you
> confirm the direction above (especially POSTFIX_COUNTED_BY_OR_NULL(len + 1)
> vs any bound expression you prefer). Default builds would remain unchanged.
Thanks for confirming my findings before my patience ran out.
I was considering to post a policy that submissions generated with
AI will be ignored unless the poster can justify the entire code.
I probably should post such a policy anyway, because submissions
with hallucinations like yours waste limited maintainer resources.
So don't do that again :-)
While doing some home work I found that adding 'counted-by' annotations
is not sufficient.
I understand that with a 'counted_by' violation, libubsan will write
a message to stderr and terminate the process. That means a Postfix
daemon process will disappear without logging any reason at all
(daemons run in the background, with stderr etc. redirected to
/dev/null).
That would be unusable.
libubsan can be told to log to a file, but Postfix client and daemon
processes run with different privileges (some privileges are
determined at runtime in local(8), virtual(8) etc.). For that reason,
logging to file would be difficult to manage and difficult to protect
- a lot of files, world-write permission, or both.
I found an undocumented interface that involves a caller-supplied
__ubsan_on_report() implementation. This function can dig up error
details and can log them with Postfix msg_warn() so that is logged
to the expected place - not scattered over a bazillion files and
not written to an insecure world-writable file. I am adding this
adapter to the Postfix library before proceeding with annotations.
You did not pick a good example. ARGV is the worst example for
introducing bounds checks. It implements a managed data type that
always checks the bound before writing to an ARGV.argv array element,
and panic()s if the check fails. A good optimizer would optimize out
the redundant 'counted_by' checks. And, there is no proper way to
verify that 'counted_by' bounds checks work, because no caller
should directly write to an ARGV.argv array element. I'd love
to see a way to make struct members read-only for non-class callers.
It could be a reason to adopt C++.
Wietse
_______________________________________________
Postfix-users mailing list -- [email protected]
To unsubscribe send an email to [email protected]
