fix: restore debug output indentation in catch clause #2134

gremat · 2025-01-17T09:54:47Z

Verbose debug output (e.g., --with-debug-verbose=2) may run into an inconsistent indentation when nested function calls are disturbed by signals (SIGBUS or SIGSEGV): the closing statement is never executed, cf. #2133 (comment) . Thanks @secDre4mer for pointing this out.

This patch restores the indentation in this case. The resulting output is obviously incomplete because it misses the closing statement but the subsequently reported error should be a sufficient hint to assess the situation. For instance,

2.256372 11010464 + yr_scanner_scan_mem(buffer=a0000004e891000 buffer_size=1048576) {
2.256429 11010464   + yr_scanner_scan_mem_blocks() {
2.256468 11010464     - _yr_get_first_block() {} = 116503290 // default iterator; single memory block, blocking
2.257259 11010464     + _yr_scanner_scan_mem_block(block_data=a0000004e891000 block->base=0x0 block->size=1048576) {
2.274888 11010464     - _yr_scanner_clean_matches() {}
2.274940 11010464   } = 4 AKA ERROR_COULD_NOT_MAP_FILE // yr_scanner_scan_mem_blocks()
2.275003 11010464 } = 4 AKA ERROR_COULD_NOT_MAP_FILE // yr_scanner_scan_mem()

does not contain the closing _yr_scanner_scan_mem_block statement.

google-cla · 2025-01-17T09:54:51Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

code-review-assist

Hi team,

I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request addresses an indentation issue in verbose debug output when nested function calls are interrupted by signals (like SIGBUS or SIGSEGV). The problem, as noted in the description, resulted in inconsistent indentation because the closing statement wasn't executed. This was originally reported in issue #2133 (comment).

Here's a breakdown of the changes:

Intent: Restore correct indentation in verbose debug output even when interrupted by signals.
Changes:
- Added a global variable yr_debug_indent to libyara/exception.h (lines 224-225). This variable is used to track the indentation level.
- Modified the YR_TRYCATCH macro in libyara/exception.h (lines 248, 263):
  - Before the jump to the catch clause, the current indentation level is stored in yr_debug_indent_before_jump (line 248).
  - In the catch clause, the indentation level is restored to the value stored before the jump (line 263).
Files Affected: libyara/exception.h (multiple changes across lines 221-267)

The updated YR_TRYCATCH macro ensures that even if a signal interrupts execution, the debug output maintains consistent indentation. While the output will be incomplete, the error message should still be informative.

I'll post a more detailed review shortly.

Debugging's a quest,
Signals may cause a slight mess,
Indentation's key.

code-review-assist

This patch addresses an indentation inconsistency in verbose debug output when signals interrupt nested function calls. The approach of restoring the indentation before the catch clause is a good way to handle the incomplete output scenario. The example provided clearly illustrates the issue and how the patch resolves it. I have a few minor suggestions regarding code style and documentation.

code-review-assist · 2025-01-17T09:56:12Z

libyara/exception.h

@@ -221,6 +221,8 @@ static void exception_handler(int sig, siginfo_t * info, void *context)
  }
 }

+extern YR_TLS int yr_debug_indent;


Consider adding a comment explaining the purpose of this variable, specifically how it's used for debug indentation. For example, you could say something like "Thread-local variable to track the indentation level for debug output."

Suggested change

extern YR_TLS int yr_debug_indent;

extern YR_TLS int yr_debug_indent; // Thread-local variable to track the indentation level for debug output.

The variable is declared elsewhere. No need to comment on it here.

code-review-assist · 2025-01-17T09:56:12Z

libyara/exception.h

@@ -243,6 +245,7 @@ typedef struct sigaction sa;
      }                                                               \
      exception_handler_usecount++;                                   \
      pthread_mutex_unlock(&exception_handler_mutex);                 \
+      int yr_debug_indent_before_jump = yr_debug_indent;              \


For consistency with the YARA coding style, consider adding a space after the = in the initialization. Also, could you clarify the purpose of saving the current indentation level with a brief comment? For example: "Save the current debug indentation level before the jump."

Suggested change

int yr_debug_indent_before_jump = yr_debug_indent; \

int yr_debug_indent_before_jump = yr_debug_indent; // Save the current debug indentation level before the jump.

Added (without breaking function-like macro).

You can ignore these AI-generated suggestions, this only an experiment and the suggestions should be taken with a grain of salt.

code-review-assist · 2025-01-17T09:56:13Z

libyara/exception.h

+        /* Restore debug output indentation in case of failure */     \
+        yr_debug_indent = yr_debug_indent_before_jump;                \


The comment could be slightly improved for clarity. How about: "Restore debug indentation to the level before the try block."

Suggested change

/* Restore debug output indentation in case of failure */ \

yr_debug_indent = yr_debug_indent_before_jump; \

/* Restore debug indentation to the level before the try block. */

yr_debug_indent = yr_debug_indent_before_jump;

The suggested extra information is redundant in the context of the (local) variable's name.

plusvic · 2025-01-17T11:01:35Z

This doesn't build properly in some CI tests.

gremat · 2025-01-17T11:19:49Z

This doesn't build properly in some CI tests.

Saw it right now. My bad, I didn't run a local build without --enable-debug. Working on a fix.

gremat · 2025-01-17T13:06:52Z

There are two CI failures left, both of them not related to my changes.

Cygwin fails because flex is missing:

And the bazel-build fails.

plusvic · 2025-01-17T14:36:00Z

libyara/exception.h

@@ -243,6 +254,8 @@ typedef struct sigaction sa;
      }                                                               \
      exception_handler_usecount++;                                   \
      pthread_mutex_unlock(&exception_handler_mutex);                 \
+      /* Save the current debug indentation level before the jump. */ \
+      YR_DEBUG_INDENT_RESET_INIT                                      \


Why not using ...

#if 0 == YR_DEBUG_VERBOSITY int yr_debug_indent_before_jump = yr_debug_indent; #endif

Instead of declaring yet another macro. It's easier to follow the code this way.

Please correct me if I'm mistaken but I think you cannot do that within YR_TRYCATCH where both macros are used. YR_TRYCATCH is a function-like macro thus, effectively, a single line (also indicated by the trailing backslashes). There, you cannot insert a construct like #if ... #endif which requires the # directives to be the first non-whitespace characters on each line.

However, I pushed a slightly different approach which uses yet another function-like macro to set yr_debug_indent in the catch clause. It might be somewhat nicer to read, but please decide for yourself. One drawback, though: we get a compiler warning (per YR_TRYCATCH instance) about an unused variable (which is only cosmetic; the compiler should optimize it away).

plusvic · 2025-01-17T14:37:12Z

libyara/exception.h

@@ -256,6 +269,9 @@ typedef struct sigaction sa;
      }                                                               \
      else                                                            \
      {                                                               \
+        /* Restore debug output indentation in case of failure */     \
+        YR_DEBUG_INDENT_RESET_RESTORE                                 \


Same here, instead of declaring YR_DEBUG_INDENT_RESET_RESTORE we could use..

#if 0 == YR_DEBUG_VERBOSITY yr_debug_indent = yr_debug_indent_before_jump; #endif

(See comment above.)

Drawback of this solution is a compiler warning about the unused variable for non-debug builds. But this should not be a performance penalty because the unused variable should be optimized away by the compiler.

code-review-assist bot reviewed Jan 17, 2025

View reviewed changes

gremat force-pushed the fix/restore-debug-output-indentation-in-catch-clause branch from 9988d9b to 3cf9194 Compare January 17, 2025 10:33

fix: restore debug output indentation in catch clause

5748522

gremat force-pushed the fix/restore-debug-output-indentation-in-catch-clause branch from 3cf9194 to 5748522 Compare January 17, 2025 11:44

plusvic requested changes Jan 17, 2025

View reviewed changes

fix: use a function-like macro to set yr_debug_indent

214c597

Drawback of this solution is a compiler warning about the unused variable for non-debug builds. But this should not be a performance penalty because the unused variable should be optimized away by the compiler.

gremat requested a review from plusvic January 29, 2025 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: restore debug output indentation in catch clause #2134

fix: restore debug output indentation in catch clause #2134

gremat commented Jan 17, 2025

google-cla bot commented Jan 17, 2025

code-review-assist bot left a comment

code-review-assist bot left a comment

code-review-assist bot Jan 17, 2025

gremat Jan 17, 2025

code-review-assist bot Jan 17, 2025

gremat Jan 17, 2025

plusvic Jan 17, 2025

code-review-assist bot Jan 17, 2025

gremat Jan 17, 2025

plusvic commented Jan 17, 2025

gremat commented Jan 17, 2025

gremat commented Jan 17, 2025

plusvic Jan 17, 2025

gremat Jan 17, 2025

plusvic Jan 17, 2025

gremat Jan 21, 2025

	extern YR_TLS int yr_debug_indent;
	extern YR_TLS int yr_debug_indent; // Thread-local variable to track the indentation level for debug output.

	int yr_debug_indent_before_jump = yr_debug_indent; \
	int yr_debug_indent_before_jump = yr_debug_indent; // Save the current debug indentation level before the jump.

		/* Restore debug output indentation in case of failure */ \
		yr_debug_indent = yr_debug_indent_before_jump; \

fix: restore debug output indentation in catch clause #2134

Are you sure you want to change the base?

fix: restore debug output indentation in catch clause #2134

Conversation

gremat commented Jan 17, 2025

google-cla bot commented Jan 17, 2025

code-review-assist bot left a comment

Choose a reason for hiding this comment

code-review-assist bot left a comment

Choose a reason for hiding this comment

code-review-assist bot Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-review-assist bot Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-review-assist bot Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plusvic commented Jan 17, 2025

gremat commented Jan 17, 2025

gremat commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment