mksh 52c bi_errorf(Tbadsubst) format string is not a string literal

Bug #1580348 reported by Chih-Hung Hsieh on 2016-05-10
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
mksh
High
Thorsten Glaser
mksh (Ubuntu)
Undecided
Unassigned

Bug Description

Lastest mksh/histrap.c (R52c) has a warning from clang/llvm compiler.
It is a tricky use of pointer to the middle of string literals,
which is recognized by gcc but not clang/llvm.

This warning now blocks mksh upgrade in Android open source.
Could you fix the following warning and other places that use
bi_errorf(Tbadsubst) or internal_errorf(Tbadsubst)?

histrap.c:220:15: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                                bi_errorf(Tbadsubst);
                                          ^~~~~~~~~
sh.h:891:19: note: expanded from macro 'Tbadsubst'
#define Tbadsubst (Tfg_badsubst + 10) /* "bad substitution" */
                        ^~~~~~~~~~~~~~~~~~

The warning can be fixed by changing
   bi_errorf(Tbadsubst);
to
   bi_errorf("%s", Tbadsubst);

If adding a few bytes is not acceptable, could you change
   bi_errorf(Tbadsubst);
to
   bi_errorf0(Tbadsubst);
where bi_errorf0 is declared as
   void bi_errorf0(const char *); // without format check
and implemented as a weak alias:
   void bi_errorf0(const char *s) __attribute__((weak, alias("bi_errorf")));

Attached file is a suggested patch to compile with Android.

Thanks.

Chih-Hung Hsieh (chh-b) wrote :
Thorsten Glaser (mirabilos) wrote :

Chih-Hung Hsieh dixit:

>Lastest mksh/histrap.c (R52c) has a warning from clang/llvm compiler.
>It is a tricky use of pointer to the middle of string literals,
>which is recognized by gcc but not clang/llvm.

Arrgh, stupid compilers, trying to be too smart and utterly failing.
I’ve recently had “fun” with newer GCC versions and those string
pooling macros, TWICE. I already decided to remove them, but for
now I’ve added GCC workarounds… turns out Clang is just as bad.

Yes, I’ll tackle this for the next release, thank you for the report.

bye,
//mirabilos
--
22:20⎜<asarch> The crazy that persists in his craziness becomes a master
22:21⎜<asarch> And the distance between the craziness and geniality is
only measured by the success 18:35⎜<asarch> "Psychotics are consistently
inconsistent. The essence of sanity is to be inconsistently inconsistent

Chih-Hung Hsieh dixit:

>change
> bi_errorf(Tbadsubst);
>to
> bi_errorf0(Tbadsubst);
>where bi_errorf0 is declared as
> void bi_errorf0(const char *); // without format check
>and implemented as a weak alias:
> void bi_errorf0(const char *s) __attribute__((weak, alias("bi_errorf")));

For the record: this is both compiler-specific (there’s no direct
use of __attribute__ in the code, and there must not be any use
that can’t be done with other compilers either) and insecure. DO
NOT DO THAT. Those strings are actually format strings and treated
as such.

bye,
//mirabilos
--
(gnutls can also be used, but if you are compiling lynx for your own use,
there is no reason to consider using that package)
 -- Thomas E. Dickey on the Lynx mailing list, about OpenSSL

Thorsten Glaser (mirabilos) wrote :

Fix committed – please do try CVS HEAD; if this doesn’t help, I will require full build logs.

Removing from the Ubuntu package, as it’s not affected by this issue; adding the upstream package.

With the last code, Build.sh (triggered by mksrc.sh in Android) will recognise LLVM/Clang automatically and set CPPFLAGS accordingly to not use our own string pooling but to rely on the compiler’s instead.

Changed in mksh (Ubuntu):
status: New → Invalid
Changed in mksh:
status: New → Fix Committed
importance: Undecided → High
assignee: nobody → Thorsten Glaser (mirabilos)
Changed in mksh:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers