search

pganalyze / libpg_query

C library for accessing the PostgreSQL parser outside of the server environment
BSD 3-Clause "New" or "Revised" License
1.21k stars 182 forks source link

strtoint doesn't work well with musl #224

Closed anuraaga closed 12 months ago

anuraaga commented 12 months ago

I have compiled this library to Wasm using wasi-sdk, which uses musl for libc. I noticed that musl seems to have an overflow related bug.

https://www.openwall.com/lists/musl/2023/12/01/1

Notably, the tests with large numbers was failing, recognizing them as ICONST

https://github.com/pganalyze/libpg_query/blob/15-latest/test/scan_tests.c#L34

I came up with a patch to workaround it by knowing what the longest integer string would be

https://github.com/wasilibs/go-pgquery/blob/main/buildtools/wasm/patch-musl.txt#L9

AFAIK, this issue is not Wasm-specific and would happen for any musl user. Would it be worth adding this patch to this project? I can send a PR if it makes sense.

anuraaga commented 12 months ago

Sorry it seems the issue may actually be Wasm specific so might not be needed here. Will double-check.

lfittl commented 12 months ago

Hmm, interesting - I don't think its great to add a custom patch for musl, mainly since we're trying to keep the diff between upstream Postgres and libpg_query as small as we can.

It appears that Postgres 16 introduced the use of pg_strtoint32_safe in this code path (as part of faff8f8e47f), which I believe should be safe from overruns (per the name):

https://github.com/postgres/postgres/blob/master/src/backend/parser/scan.l#L1372

Maybe we should revisit whether this is a problem on the 16 branch? (see #220, expecting to have this available soon)

anuraaga commented 12 months ago

Thanks for the pointer, indeed it seems worth seeing how it works with postgres 16. Will close this for now since looks like it should be fine then.