Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't create field name of more than 7 characters on Mac #136

Closed
ChrisJamesC opened this issue Jun 28, 2017 · 3 comments
Closed

Can't create field name of more than 7 characters on Mac #136

ChrisJamesC opened this issue Jun 28, 2017 · 3 comments

Comments

@ChrisJamesC
Copy link

ChrisJamesC commented Jun 28, 2017

When running this command I get a crash with Abort trap: 6:

tdb make -f uuid,time,username -o bob

However, if the 8 characters field is not the first, it works:

tdb make -f uuid,time,hello,username -o bob

I tried installing traildb manually or with Homebrew with the same results.

I found this bug because the first tutorial has a field with 'username' and it made my program crash.

Machine configuration:
Macbook pro retina
Processor: 2.9 GHz Intel Core i5
OS: macOS Sierra (10.12.5)

Note that I tried on a mac which didn't run on Sierra and tdb worked fine.

@ChrisJamesC ChrisJamesC changed the title Can't create column of more than 7 characters Can't create field of more than 7 characters Jun 28, 2017
@ChrisJamesC ChrisJamesC changed the title Can't create field of more than 7 characters Can't create field name of more than 7 characters on Mac Jul 6, 2017
@knutin
Copy link
Member

knutin commented Jul 7, 2017

I'm able to reproduce this on Sierra and it looks to me like it crashes inside Judy.

(lldb) target create "tdb"
Current executable set to 'tdb' (x86_64).
(lldb) settings set -- target.run-args  "make" "-f" "uuid,time,username" "-o" "bob"
(lldb) run
Process 38569 launched: '/usr/local/bin/tdb' (x86_64)
2017-07-07 17:12:10.411021+0100 tdb[38569:5691526] detected buffer overflow
Process 38569 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x00007fffbadced42 libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fffbadced42 <+10>: jae    0x7fffbadced4c            ; <+20>
    0x7fffbadced44 <+12>: movq   %rax, %rdi
    0x7fffbadced47 <+15>: jmp    0x7fffbadc7caf            ; cerror_nocancel
    0x7fffbadced4c <+20>: retq
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x00007fffbadced42 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fffbaebc457 libsystem_pthread.dylib`pthread_kill + 90
    frame #2: 0x00007fffbad34420 libsystem_c.dylib`abort + 129
    frame #3: 0x00007fffbad34592 libsystem_c.dylib`abort_report_np + 181
    frame #4: 0x00007fffbad5af28 libsystem_c.dylib`__chk_fail + 48
    frame #5: 0x00007fffbad5aef8 libsystem_c.dylib`__chk_fail_overflow + 16
    frame #6: 0x00007fffbad5b145 libsystem_c.dylib`__strcpy_chk + 83
    frame #7: 0x0000000100116bc5 libtraildb.0.dylib`JudySLIns + 942
    frame #8: 0x00000001000ede41 libtraildb.0.dylib`tdb_cons_open + 129
    frame #9: 0x0000000100004165 tdb`op_make + 901
    frame #10: 0x0000000100002e1f tdb`main + 927
    frame #11: 0x00007fffbaca0235 libdyld.dylib`start + 1
    frame #12: 0x00007fffbaca0235 libdyld.dylib`start + 1```

I'll try looking a bit deeper. Maybe we're using Judy incorrectly on this platform.

@knutin
Copy link
Member

knutin commented Aug 15, 2017

Okay, here's what I've found:

  • In strcpy bult with clang on Mac OS X, there's a call to __strcpy_chk. I'm not sure why or how this is inserted, but since we're not crashing on Linux with Judy built with clang I'm assuming it's platform-specific.

  • __strcpy_chk ensures that the destination buffer is large enough to hold the source. I'm not 100% clear on exactly how it does it, but in the examples I could find it looks it can reliably find the size of a fixed buffer, ie. char buf[8]. I don't think it would work for a buffer allocated at runtime, but I might be wrong in that.

  • Judy calls strcpy to copy the value we want to insert into a variable-sized field on a struct. (The last field of a struct can be variable in size). Here's the definition of said struct, from src/JudySL/JudySL.c:

typedef struct SHORCUTLEAF
{
    Pvoid_t   scl_Pvalue;               // callers value area.
    uint8_t   scl_Index[WORDSIZE];      // base Index string.
} scl_t  , *Pscl_t;
  • Since scl_Index is defined to be WORDSIZE, which on most machines is now 8, __strcpy_chk will think the destination can only hold 8 bytes, even though Judy correctly allocates the appropriate amount of memory to allow the value to "spill over" past the end of the struct. At this point we crash.

  • (If the first field is less than 8 bytes, the second field can be more than 8 bytes. I can't yet explain why this happens)

As to the solution, I see three options:

  1. Figure out if we can trick Judy into being compiled without __strcpy_chk on Mac OS X. We have a brew tap for this package, so it's feasible.

  2. Patch scl_t in JudySL.c to work with __strcpy_chk, presumably by not defining the size, or setting it to 0 or something similar.

  3. This particular code path is only used to ensure the fields as input to tdb_cons are unique. We could solve this in a different way, for example by sorting the fields and checking for duplicates.

@tuulos
Copy link
Member

tuulos commented Aug 15, 2017

__strcpy_chk seems to originate from source code fortification. We could try to disable it e.g. with -D_FORTIFY_SOURCE=0. Maybe -fno-builtin would work too.

If a compiler flag fixes it, we can include the flag in our tap.

knutin added a commit to knutin/homebrew-judy that referenced this issue Sep 15, 2017
On Mac OS, strcpy under the hood calls `__strcpy_chk` which attempts
to check if the destination is large enough to fit the input. Due to
how the `SHORTCUTLEAF` struct is defined in `JudySL.c`, `__strcpy_chk`
believes it can only fit `sizeof(WORDSIZE)`, while it is actually a
variable-sized field.

This change passes `_FORTIFY_SOURCE=0` to clang, which ensures
`__strcpy_chk` is *not* inserted.

Fixes traildb/traildb#136
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants