Skip to content

Commit

Permalink
[#15682] YSQL: Fix stack_is_too_deep function in ASAN
Browse files Browse the repository at this point in the history
Summary:
The `stack_is_too_deep` Postgres function is used to limits stack depth based on total stack size.
The stack size is calculated by using  local variables address offset.
This method works well in case of regular call stack (i.e. when all stack frames are allocated in stack).
But for the `detect_stack_use_after_return` ASAN uses fake stack.
In case of using it stack frames are allocated in the heap.
As a result it is not possible to estimate stack depth base on local variables address offset and the `stack_is_too_deep` returns almost unpredictable results.

To make `stack_is_too_deep `return predictable result in case of ASAN it is reasonable to return `false` all the time.

**Note**:
YSQL has some unit tests which checks that Postgres can detect too deep recursion.
These tests changes the `max_stack_depth` GUC variable to lower value.
And later restore the original value with the `RESET max_stack_depth` statement.
To make these tests works under the ASAN the function returns true in case the `max_stack_depth`
GUC contains non default value and number of call stack frames is huge enough.
The check of call stack frames is required to avoid undesired failure on attempt to restore
original value for the `max_stack_depth` GUC with the `RESET max_stack_depth` statement.
Jira: DB-5047

Test Plan:
Jenkins

```
./yb_build.sh asan --java-test org.yb.pgsql.TestPgRegressJson#testPgRegressJson
```

Reviewers: loginov, jason

Reviewed By: loginov, jason

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D35979
  • Loading branch information
d-uspenskiy committed Jun 26, 2024
1 parent 411a32e commit c2e13ef
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 0 deletions.
39 changes: 39 additions & 0 deletions src/postgres/src/backend/tcop/postgres.c
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@

/* YB includes */
#include "replication/walsender_private.h"
#include "utils/guc_tables.h"

/* ----------------
* global variables
Expand Down Expand Up @@ -3324,6 +3325,44 @@ check_stack_depth(void)
bool
stack_is_too_deep(void)
{
#ifdef ADDRESS_SANITIZER
// Postgres analyzes/limits stack depth based on local variables address
// offset.
// This method works well in case of regular call stack (i.e. when all
// stack frames are allocated in stack).
// But for the detect_stack_use_after_return ASAN uses fake stack. In case
// of using it stack frames are allocated in the heap. As a result it is
// not possible to estimate stack depth base on local variables address
// offset.
// To make stack_is_too_deep return predictable results in case of ASAN it
// is reasonable to return false all the time.
// Note:
// YSQL has some unit tests which checks that Postgres can detect too
// deep recursion. These tests change the `max_stack_depth` GUC variable
// to lower value. And later restore the original value with the
// `RESET max_stack_depth` statement.
// To make these tests works under the ASAN the function returns true in
// case the `max_stack_depth` GUC contains non default value and number of
// call stack frames is huge enough.
// The check of call stack frames is required to avoid undesired failure on
// attempt to restore original value for the `max_stack_depth` GUC with
// the `RESET max_stack_depth` statement.
if (get_guc_variables()) {
const char* max_stack_depth_GUC = "max_stack_depth";
const char* current_value =
GetConfigOption(max_stack_depth_GUC, false, false);
const char* default_value =
GetConfigOptionResetString(max_stack_depth_GUC);
if (strcmp(current_value, default_value) != 0) {
static const int MAX_STACK_FRAMES = 64;
void* frames[MAX_STACK_FRAMES];
int frames_count =
YBCGetCallStackFrames(frames, MAX_STACK_FRAMES, 0);
return frames_count >= MAX_STACK_FRAMES;
}
}
return false;
#endif
char stack_top_loc;
long stack_depth;

Expand Down
5 changes: 5 additions & 0 deletions src/yb/yql/pggate/util/ybc_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "yb/util/random_util.h"
#include "yb/util/scope_exit.h"
#include "yb/util/status_format.h"
#include "yb/util/stack_trace.h"
#include "yb/util/thread.h"

#include "yb/yql/pggate/util/ybc-internal.h"
Expand Down Expand Up @@ -486,6 +487,10 @@ uint8_t YBCGetQueryIdForCatalogRequests() {
return static_cast<uint8_t>(ash::FixedQueryId::kQueryIdForCatalogRequests);
}

int YBCGetCallStackFrames(void** result, int max_depth, int skip_count) {
return google::GetStackTrace(result, max_depth, skip_count);
}

} // extern "C"

} // namespace yb::pggate
2 changes: 2 additions & 0 deletions src/yb/yql/pggate/util/ybc_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,8 @@ const char* YBCGetWaitEventComponent(uint32_t wait_event_info);
const char* YBCGetWaitEventType(uint32_t wait_event_info);
uint8_t YBCGetQueryIdForCatalogRequests();

int YBCGetCallStackFrames(void** result, int max_depth, int skip_count);

#ifdef __cplusplus
} // extern "C"
#endif

0 comments on commit c2e13ef

Please sign in to comment.