··· diff -up ./src/conf_eval.c.fix ./src/conf_eval.c --- ./src/conf_eval.c.fix 2023-12-22 12:12:22.961141634 +0100 +++ ./src/conf_eval.c 2023-12-22 14:09:21.217786675 +0100 @@ -166,6 +166,7 @@ static DB_ATTR_TYPE eval_attribute_expre static void set_database_attr_option(DB_ATTR_TYPE attr, int linenumber, char *filename, char* linebuf) { char *str; + long num; DB_ATTR_TYPE hashes = get_hashes(true); if (attr&(~hashes)) { @@ -298,8 +299,20 @@ static void eval_config_statement(config LOG_CONFIG_FORMAT_LINE(LOG_LEVEL_CONFIG, "set 'config_version' option to '%s'", str) break; case VERBOSE_OPTION: - log_msg(LOG_LEVEL_ERROR, "%s:%d: 'verbose' option is no longer supported, use 'log_level' and 'report_level' options instead (see man aide.conf for details) (line: '%s')", conf_filename, conf_linenumber, conf_linebuf); - exit(INVALID_CONFIGURELINE_ERROR); + log_msg(LOG_LEVEL_CONFIG, "%s:%d: 'verbose' option is deprecated, use 'log_level' and 'report_level' options instead (see man aide.conf for details) (line: '%s')", conf_filename, conf_linenumber, conf_linebuf); + str = eval_string_expression(statement.e, linenumber, filename, linebuf); + num = strtol(str, NULL, 10); + + if (num < 0 && num > 255) { + LOG_CONFIG_FORMAT_LINE(LOG_LEVEL_ERROR, "invalid verbose level: '%s'", str); + exit(INVALID_CONFIGURELINE_ERROR); + } + + if (num >= 10) { + set_log_level(LOG_LEVEL_DEBUG); + } + + free(str); break; case LIMIT_CMDLINE_OPTION: /* command-line options are ignored here */ ··· The patch contains a scoping issue with the `num` variable, which is defined in one function (`set_database_attr_option`) but used in another (`eval_config_statement`), causing a compilation error. Additionally, the condition `if (num < 0 && num > 255)` is logically incorrect as `num` cannot simultaneously be less than 0 and greater than 255. Reproducible: Always Steps to Reproduce: 1.Apply the aide-verbose.patch to the aide package. 2.Build and configure the package. 3.Run the application or compile the code Actual Results: - The code fails to compile due to the scoping issue with the num variable being used in a different function (eval_config_statement) from where it is defined (set_database_attr_option). - Additionally, the condition if (num < 0 && num > 255) is logically incorrect. Expected Results: - The code should compile successfully without errors. - The condition should correctly check if num is outside the valid range 1.This issue occurs specifically in the aide-verbose.patch for the aide package，the patch introduces a scoping error for the num variable and an incorrect condition in the eval_config_statement function. 2.In the spec file, there is a ’%patch -R -P 1 -p1 -b .verbose’ command, which seems to indicate that the patch is being reverted. I’m a bit unclear about this step. Could you please clarify its specific purpose? Will the aide-verbose.patch patch be applied correctly despite this?
Created attachment 2076827 [details] aide-verbose.patch
Hello, I think you misconfigured the needinfo as I am not the maintainer.
> 2.In the spec file, there is a ’%patch -R -P 1 -p1 -b .verbose’ command, which seems to indicate that the patch is being reverted. I’m a bit unclear about this step. Could you please clarify its specific purpose? Will the aide-verbose.patch patch be applied correctly despite this? So, this is quite weird. It does seem like the patch is "applied" in such a way that the bad code does not appear in the file. If you run `fedpkg prep` on the repo and then examine aide-0.18.8-build/aide-0.18.8/src/conf_eval.c , it does not have the bad code in it; there is a aide-0.18.8-build/aide-0.18.8/src/conf_eval.c.verbose that *does* have the bad code in it, and that 'should be' a backup of the original file before patching (that's what the `-b .verbose` means). But...if you examine the file in the tarball, it doesn't have the bad code in it either. And if you remove the Patch1 line and the %patch line from the spec and run `fedpkg prep` and check the file, again, it doesn't have the bad code in it. So...I don't understand how the patch command is actually succeeding, or where the "backup file" with the bad code in it comes from. It's rather weird. Maybe that's just how patch -R -b works, but if so, that...sure is an odd way to work... Anyway, I don't think this patch is actually breaking the package as it stands, but it is weird and confusing and doesn't appear to serve any purpose, so it should probably be removed... The patch was added when aide was 0.18.6, not 0.18.8, but that doesn't seem to explain anything; the 'bad' code wasn't in 0.18.6 either.
This is indeed quite weird, but putting aside how the project applied this patch, is there an issue with the patch itself? Why hasn't the variable scope issue affected the compilation?
That is questionable packaging. First the patch is applied as part of "%autosetup" then it is applied a second time reverse by calling %patch manually. No comment in the spec file that explains the goal.
Oh, duh, of course - that's what happens. Thanks, Michael. I even ran into autosetup applying the patch forwards while I was investigating this, but somehow didn't add up that that's what happens in the real build. So this is what happens: 1. We start with good code from upstream 2. %autosetup applies the patch "forwards" (temporarily putting the code in the 'bad' state) 3. %patch -R -P 1 -p1 -b .verbose immediately reverts it, so we're back at 1, good code from upstream 4. We run the build So the build actually happens with unmodified upstream 0.18.8, which is why it doesn't fail. This is what I was trying to explain in comment #3 - we don't actually wind up trying to build the code as it would be with that patch applied 'forwards'. You might think we got here via some kinda weird multi-step process, but no. %autosetup in the spec predates the addition of the patch by a long way. The patch and the %patch line were both added in the same commit, https://src.fedoraproject.org/rpms/aide/c/a003ad04cf2504564c0497c9cce5e5aecb9d601b?branch=rawhide , which just says "Fix verbose option". The patch isn't in git format so doesn't have an author or a commit message, and no comment explaining it was added to the spec (this is all against best practice). The idea of the patch is to keep support for some kind of config setting upstream wanted to just entirely drop support for. I think the `if (num < 0 && num > 255) {` line is meant to be `if (num < 0 || num > 255) {` (though there's really no obvious reason it needs to exist, the later check could just be `if (num >= 10 && num < 255) {`. But it's a questionable patch anyway; we shouldn't be second-guessing upstream's choices like that without a clear justification, which should've been put in the spec file.
I went ahead and dropped the patch as it's clearly nonsensical - https://src.fedoraproject.org/rpms/aide/c/3073404dcdbb82446c5844ac4bca68797a1763d6?branch=rawhide . If the maintainer wants to re-introduce this, they can, but please make sure: 1) it actually works (address all the issues Peng noticed) 2) the reason for doing it is explained in the spec file and/or the commit message 3) the status of the patch wrt to upstream is also explained (has it been submitted upstream? if not, why not?)