Bug#975650: blhc: reports false positives for missing flags
On Tue, Oct 05, 2021 at 09:32:21PM +0200, Fabian Wolff wrote: > On 10/5/21 1:48 PM, Simon Ruderich wrote: >> Could you test the attached patch and tell me if this works for >> you for real builds? > > Thankfully, I still had the full log file lying around in which I > originally discovered this issue, and I am indeed no longer getting > any false positives with your patch. (I don't know whether this causes > any new false negatives, but I suppose you have other tests for this?) > > Thanks for your work on this! Hello Fabian, thanks for testing. I've just released blhc 0.13 with the fix. I don't that it should cause false negatives. Regards Simon -- + Privatsphäre ist notwendig + Ich verwende GnuPG http://gnupg.org + Öffentlicher Schlüssel: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
Bug#975650: blhc: reports false positives for missing flags
On 10/5/21 1:48 PM, Simon Ruderich wrote: > Could you test the attached patch and tell me if this works for > you for real builds? Thankfully, I still had the full log file lying around in which I originally discovered this issue, and I am indeed no longer getting any false positives with your patch. (I don't know whether this causes any new false negatives, but I suppose you have other tests for this?) Thanks for your work on this! Best, Fabian
Bug#975650: blhc: reports false positives for missing flags
On Sun, Nov 29, 2020 at 07:21:48PM +0100, Fabian Wolff wrote:
> On 11/28/20 10:47 PM, Eriberto wrote:
>> Thanks Fabian and Simon!
>>
>> Considering that some false positives can't be fixed in blhc source
>> code, could I close this bug?
>
> I think it would make more sense to close this bug with the next
> upstream release, given that some changes in blhc have been applied
> because of it (commit 79d3a9e in the upstream repository), and it's
> also mentioned in the NEWS entry for the next release.
Hello Fabian,
sorry for the late reply. While looking at Olaf's report I also
thought about your original report and I might have a (partial)
solution.
The problem in this case is only the compiler detection, not
actually handling environment variables. So stripping them should
be enough. I've implemented a patch which strips (basic)
environment variables (no nested quotes, command substitution,
etc.).
Could you test the attached patch and tell me if this works for
you for real builds?
Regards
Simon
--
+ Privatsphäre ist notwendig
+ Ich verwende GnuPG http://gnupg.org
+ Öffentlicher Schlüssel: 0x92FEFDB7E44C32F9
From 5cb3ea785d8c4602a703336797f42295d1980827 Mon Sep 17 00:00:00 2001
Message-Id: <5cb3ea785d8c4602a703336797f42295d1980827.1633434227.git.si...@ruderich.org>
From: Simon Ruderich
Date: Tue, 5 Oct 2021 13:43:29 +0200
Subject: [PATCH] Strip (basic) environment variables before compiler detection
---
bin/blhc | 20 +++-
t/tests.t | 4 ++--
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/bin/blhc b/bin/blhc
index 2f8da5f..d41ff88 100755
--- a/bin/blhc
+++ b/bin/blhc
@@ -1022,9 +1022,27 @@ foreach my $file (@ARGV) {
$complete_line = undef;
}
+my $noenv = $line;
+# Strip (basic) environment variables for compiler detection.
+# Nested quotes, command substitution, etc. is not supported.
+$noenv =~ s/^
+\s*
+(?:
+[a-zA-Z_]+ # environment variable name
+=
+(?:
+[^\s"'\$\\]+# non-quoted string
+|
+'[^"'\$`\\]*' # single-quoted string
+|
+"[^"'\$`\\]*" # double-quoted string
+)
+\s+
+)*
+//x;
# Ignore lines with no compiler commands.
next if not $non_verbose
-and not $line =~ /$cc_regex_normal/o;
+and not $noenv =~ /$cc_regex_normal/o;
# Ignore lines with no filenames with extensions. May miss some
# non-verbose builds (e.g. "gcc -o test" [sic!]), but shouldn't be
# a problem as the log will most likely contain other non-verbose
diff --git a/t/tests.t b/t/tests.t
index b4c0352..737f3ec 100644
--- a/t/tests.t
+++ b/t/tests.t
@@ -633,8 +633,8 @@ CPPFLAGS missing (-D_FORTIFY_SOURCE=2): gcc -g -O2 -fstack-protector-strong -Wfo
LDFLAGS missing (-fPIE -pie -Wl,-z,relro -Wl,-z,now): gcc -g -O2 -fstack-protector-strong -Wformat -Wformat-security -Werror=format-security test.c -o lib`basename test/test`.so
';
-is_blhc 'env', '--all', 8,
-'CPPFLAGS missing (-D_FORTIFY_SOURCE=2): VERSION=v-amd64-linux CPP="gcc -x assembler-with-cpp -E -P -Wdate-time -D_FORTIFY_SOURCE=2" CPPFLAGS="-Wdate-time -D_FORTIFY_SOURCE=2" ../../config/gen-posix-names.sh _SC_ ml_sysconf.h
+is_blhc 'env', '--all', 1,
+'No compiler commands!
';
--
2.33.0
signature.asc
Description: PGP signature
Bug#975650: blhc: reports false positives for missing flags
On Sun, Feb 21, 2021 at 03:24:26PM -0500, Olek Wojnar wrote: > I have run into this exact issue with bazel-bootstrap builds. [1] I love > what blhc does so I'd rather not disable it due to these false positives, > but I also like for the Salsa CI to let me know when a recent commit has > caused a problem, and constant test failures mask that. > > Would it be possible to change the regex so that it will also accept a " or > a ' next to the spaces surrounding the flag? That way all of the three > following would be considered valid: > -D_FORTIFY_SOURCE=2 > '-D_FORTIFY_SOURCE=2' > "-D_FORTIFY_SOURCE=2" Hello Olek, sorry for the late reply. The issue you mentioned is different from the originally reported bug and much easier to fix. Should be fixed with [1], please check and report back. The original issue is that blhc treats the line as a compiler line because it sees the "gcc" in the environment variable (not noticing that it's just an environment variable and not a command). > Thanks for your work on this package, it's a very useful indicator of > regressions or other coding issues! You're welcome! Regards Simon [1]: https://ruderich.org/simon/gitweb/?p=blhc/blhc.git;a=commitdiff;h=06b7783ef223d0f58804f3f08d27c45dc3b97351 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
Bug#975650: blhc: reports false positives for missing flags
Hello Simon and other interested parties, On Sat, 28 Nov 2020 12:28:16 +0100 Simon Ruderich wrote: > > is the problem because blhc requires a space after each flag. I > know this handling is not perfect but parsing arbitrary shell > commands is difficult. I have run into this exact issue with bazel-bootstrap builds. [1] I love what blhc does so I'd rather not disable it due to these false positives, but I also like for the Salsa CI to let me know when a recent commit has caused a problem, and constant test failures mask that. Would it be possible to change the regex so that it will also accept a " or a ' next to the spaces surrounding the flag? That way all of the three following would be considered valid: -D_FORTIFY_SOURCE=2 '-D_FORTIFY_SOURCE=2' "-D_FORTIFY_SOURCE=2" If you think it could cause problems to implement that as default behavior, is it something that could be implemented as an option? Something along the lines of `blhc --accept-quoted` perhaps? Thanks for your work on this package, it's a very useful indicator of regressions or other coding issues! -Olek [1] https://salsa.debian.org/bazel-team/bazel-bootstrap/-/jobs/1452282
Bug#975650: blhc: reports false positives for missing flags
On 11/28/20 10:47 PM, Eriberto wrote: > Thanks Fabian and Simon! > > Considering that some false positives can't be fixed in blhc source > code, could I close this bug? I think it would make more sense to close this bug with the next upstream release, given that some changes in blhc have been applied because of it (commit 79d3a9e in the upstream repository), and it's also mentioned in the NEWS entry for the next release.
Bug#975650: blhc: reports false positives for missing flags
Thanks Fabian and Simon! Considering that some false positives can't be fixed in blhc source code, could I close this bug? Cheers, Eriberto
Bug#975650: blhc: reports false positives for missing flags
On 11/28/20 12:28 PM, Simon Ruderich wrote: > blhc uses a few heuristics to detect lines with (possible) > compiler commands to prevent false negatives. Lines containing > `gcc` and similar are flagged. In this case the CC= environment > variable triggers this. Then blhc looks for the actual build > command and its flags. Here the quote after -D_FORTIFY_SOURCE=2 > is the problem because blhc requires a space after each flag. I > know this handling is not perfect but parsing arbitrary shell > commands is difficult. I see. Thanks for your explanation! I'm aware of the complexity and difficulty of parsing arbitrary shell commands, so it's not a problem at all that some false positives (and negatives) exist. I just found this particular example confusing because blhc seemed to recognize the compiler command in the environment variable assignment, but not the flags on the same line, which seemed weird to me. Thanks again for having a look at this! Fabian
Bug#975650: blhc: reports false positives for missing flags
On Tue, Nov 24, 2020 at 05:16:03PM +0100, Fabian Wolff wrote: > Dear maintainer, > > consider the following warnings emitted by blhc (line breaks are mine; > see the attached "test.log" file for an input that reproduces this > problem): > > [snip] Hello Fabian, thanks for the sample log to easily replicate the issue. > One could argue whether blhc should even consider lines like these, > because they do not actually contain any compiler calls; perhaps it > would be more sensible to delay the warnings to the actual compiler > calls that the recursive make or the script perform. However, it > doesn't hurt to be on the safe side and check calls like these, too. > > The problem is that the reportedly missing flags aren't missing at > all: The former call contains "-Wl,-z,relro" in both CFLAGS and > LDFLAGS; the latter call contains "-D_FORTIFY_SOURCE=2" in both CPP > and CPPFLAGS. > > So, why does blhc think that those flags are missing? blhc uses a few heuristics to detect lines with (possible) compiler commands to prevent false negatives. Lines containing `gcc` and similar are flagged. In this case the CC= environment variable triggers this. Then blhc looks for the actual build command and its flags. Here the quote after -D_FORTIFY_SOURCE=2 is the problem because blhc requires a space after each flag. I know this handling is not perfect but parsing arbitrary shell commands is difficult. I've just pushed a fix for the `make` case which is easily fixable without causing false negatives (all make commands are now simply ignored). In addition, I improved the line splitting so something like `make && gcc test.c` now correctly triggers a warning (in the past lines were split only on ";", now "&&" and "||" are also considered; I hope this won't cause too many false positives). However, the second command is more difficult to fix because it would require stripping all environment variables from each command. Although possible it would be quite slow and complex (escaping and nested quotes must be considered). I think in this case the simplest solution is to manually ignore the line with "blhc: ignore-line-regexp: REGEXP" in debian/rules. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 signature.asc Description: PGP signature
Bug#975650: blhc: reports false positives for missing flags
Hi Eriberto, thanks for your quick reply! On 11/24/20 5:59 PM, Eriberto wrote: > Since 0.12 version, blhc is able to ignore false positives spotted by > line(s) "injected" inside .build file via debian/rules. See more > details in blhc(1) manpage. There are examples in > /usr/share/doc/blhc/README.Debian. Please, check also the debian/rules > files in blhc, ngetty, libinsane, calibre, dxvk and picard-tools. > > Please, let me know if I can close this bug. Well, just because you can work around it doesn't mean that the bug is fixed, right? I'm trying to help improve blhc, so that false positives such as the ones I was describing ideally don't even arise. Let's wait to see what Simon thinks of this; if he deems it irrelevant and/or doesn't want to fix it, then you can close the bug. Best, Fabian
Bug#975650: blhc: reports false positives for missing flags
Hi Fabian, Em ter., 24 de nov. de 2020 às 13:27, Fabian Wolff escreveu: > > Dear maintainer, > > consider the following warnings emitted by blhc (line breaks are mine; > see the attached "test.log" file for an input that reproduces this > problem): > > LDFLAGS missing (-Wl,-z,relro): make VERSION="v-amd64-linux" MAKE="make" \ > CC="gcc -std=gnu99 -Wall" CFLAGS="-O2 -m64 -g -O2 \ > -fdebug-prefix-map=/<>=. -fstack-protector-strong\ > -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 \ > -Wl,-z,relro" DEFS="-DARCH_AMD64 -DSIZE_64 -DOPSYS_UNIX -DOPSYS_LINUX \ > -D_GNU_SOURCE -DGNU_ASSEMBLER -DDLOPEN -DINDIRECT_CFUNC" \ > CPPFLAGS="-Wdate-time -D_FORTIFY_SOURCE=2" LDFLAGS="-Wl,-z,relro" \ > AR="ar" ARFLAGS="rcv" RANLIB="ranlib" INCLUDES="-I../../objs \ > -I../../include -I.." libposix-os.a) > > CPPFLAGS missing (-D_FORTIFY_SOURCE=2): VERSION=v-amd64-linux \ > CPP="gcc -x assembler-with-cpp -E -P -Wdate-time -D_FORTIFY_SOURCE=2" \ > CPPFLAGS="-Wdate-time -D_FORTIFY_SOURCE=2"\ > ../../config/gen-posix-names.sh _SC_ ml_sysconf.h > > One could argue whether blhc should even consider lines like these, > because they do not actually contain any compiler calls; perhaps it > would be more sensible to delay the warnings to the actual compiler > calls that the recursive make or the script perform. However, it > doesn't hurt to be on the safe side and check calls like these, too. > > The problem is that the reportedly missing flags aren't missing at > all: The former call contains "-Wl,-z,relro" in both CFLAGS and > LDFLAGS; the latter call contains "-D_FORTIFY_SOURCE=2" in both CPP > and CPPFLAGS. Since 0.12 version, blhc is able to ignore false positives spotted by line(s) "injected" inside .build file via debian/rules. See more details in blhc(1) manpage. There are examples in /usr/share/doc/blhc/README.Debian. Please, check also the debian/rules files in blhc, ngetty, libinsane, calibre, dxvk and picard-tools. Please, let me know if I can close this bug. Cheers, Eriberto
Bug#975650: blhc: reports false positives for missing flags
Package: blhc Version: 0.12-2 Severity: normal X-Debbugs-CC: si...@ruderich.org Dear maintainer, consider the following warnings emitted by blhc (line breaks are mine; see the attached "test.log" file for an input that reproduces this problem): LDFLAGS missing (-Wl,-z,relro): make VERSION="v-amd64-linux" MAKE="make" \ CC="gcc -std=gnu99 -Wall" CFLAGS="-O2 -m64 -g -O2 \ -fdebug-prefix-map=/<>=. -fstack-protector-strong\ -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 \ -Wl,-z,relro" DEFS="-DARCH_AMD64 -DSIZE_64 -DOPSYS_UNIX -DOPSYS_LINUX \ -D_GNU_SOURCE -DGNU_ASSEMBLER -DDLOPEN -DINDIRECT_CFUNC" \ CPPFLAGS="-Wdate-time -D_FORTIFY_SOURCE=2" LDFLAGS="-Wl,-z,relro" \ AR="ar" ARFLAGS="rcv" RANLIB="ranlib" INCLUDES="-I../../objs \ -I../../include -I.." libposix-os.a) CPPFLAGS missing (-D_FORTIFY_SOURCE=2): VERSION=v-amd64-linux \ CPP="gcc -x assembler-with-cpp -E -P -Wdate-time -D_FORTIFY_SOURCE=2" \ CPPFLAGS="-Wdate-time -D_FORTIFY_SOURCE=2"\ ../../config/gen-posix-names.sh _SC_ ml_sysconf.h One could argue whether blhc should even consider lines like these, because they do not actually contain any compiler calls; perhaps it would be more sensible to delay the warnings to the actual compiler calls that the recursive make or the script perform. However, it doesn't hurt to be on the safe side and check calls like these, too. The problem is that the reportedly missing flags aren't missing at all: The former call contains "-Wl,-z,relro" in both CFLAGS and LDFLAGS; the latter call contains "-D_FORTIFY_SOURCE=2" in both CPP and CPPFLAGS. So, why does blhc think that those flags are missing? Best regards, Fabian dpkg-buildpackage: info: host architecture amd64 (cd ../c-libs/posix-os; make VERSION="v-amd64-linux" MAKE="make" CC="gcc -std=gnu99 -Wall" CFLAGS="-O2 -m64 -g -O2 -fdebug-prefix-map=/<>=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -Wl,-z,relro" DEFS="-DARCH_AMD64 -DSIZE_64 -DOPSYS_UNIX -DOPSYS_LINUX -D_GNU_SOURCE -DGNU_ASSEMBLER -DDLOPEN -DINDIRECT_CFUNC" CPPFLAGS="-Wdate-time -D_FORTIFY_SOURCE=2" LDFLAGS="-Wl,-z,relro" AR="ar" ARFLAGS="rcv" RANLIB="ranlib" INCLUDES="-I../../objs -I../../include -I.." libposix-os.a) VERSION=v-amd64-linux CPP="gcc -x assembler-with-cpp -E -P -Wdate-time -D_FORTIFY_SOURCE=2" CPPFLAGS="-Wdate-time -D_FORTIFY_SOURCE=2" ../../config/gen-posix-names.sh _PC_ ml_pathconf.h
