Update WAF to 1.15.0 #39
Conversation
There was a problem hiding this comment.
First look at it, a couple of nits:
- Can you add tests that use permanent and ephemeral addresses at the same time?
- Can you put the right permissions back on the embedded files ? (755 instead of 644)
- The embedded libddwaf on OSX where named with
_before for a very specific reason but I can't reproduce it since I don't have a Mac anymore, let's keep it in the back of our mind for now. - As you can see in the
run()function, we keep track of the ddwaf_object parameter in the context struct because it is required for them to live all the way to the end of the context. My guess is that this is not the case for ephemeral addresses. Can you take a look at this with @Anilm3 ?
Ephemeral addresses are freed at the end of |
Why'd they need to be executable? They're embedded as a byte slice in another file, and the file permissions won't be persisted when doing that anyway? |
# Conflicts: # context.go # decoder.go # include/ddwaf.h # lib/darwin-amd64/_libddwaf.dylib # lib/darwin-arm64/_libddwaf.dylib # lib/linux-amd64/libddwaf.so # lib/linux-arm64/libddwaf.so # waf.go # waf_dl.go # waf_dl_unsupported.go # waf_test.go
RomainMuller
force-pushed
the
rmuller/waf-1.15.0
branch
from
eee8ad0 to
8caaf80
Compare
RomainMuller
force-pushed
the
rmuller/waf-1.15.0
branch
from
0488102 to
f39c650
Compare
@RomainMuller This is very useful to link the library directly when debugging with a small C script. That's usually why we need to put it in 755. But more than that, it's to avoid dealing with the wierdest errors coming from the linker trying to access the library ( |
That I can understand... even though this sounds like a micro-improvement.
That is only relevant when trying to directly link to those files locally, and won't change a thing with the embed (the permissions of the file we dump in a temporary location is what matters here). |
| @@ -85,12 +113,12 @@ func (context *Context) Run(addressesToData map[string]any, timeout time.Duratio | |||
|
|
|||
| // Save the Go pointer references to addressesToData that were referenced by the encoder | |||
| // into C ddwaf_objects. libddwaf's API requires to keep this data for the lifetime of the ddwaf_context. | |||
| defer context.cgoRefs.append(encoder.cgoRefs) | |||
| defer context.cgoRefs.append(persistentEncoder.cgoRefs) | |||
There was a problem hiding this comment.
What allows us not to append ephemeralData.cgoRefs?
IIUC, we can avoid it because the Go reference is kept by this function call.
But if that's the case, I would try to ensure it even more explicitly to avoid potential compiler optimizations somehow removing the reference. WDYT?
There was a problem hiding this comment.
I didn't find any test on ephemeral addresses and that might be why this problem wasn't caught.
Can you please add this new type of entry in the TestConcurrency tests? This is the test suite that allowed us to find all the issues we had with the purego port.
There was a problem hiding this comment.
There are definitely tests on ephemeral addresses. Not in the concurrent suite (but I also am not sure how this would uncover anything the other test hasn't).
Yeah - these were copied from surrounding style... I did not want to do a global replace as it would have further inflated the diff size... |
Also re-wrote the
update.shto be in go, for improved expressiveness. This includes dropping thestrippass as themusllinux images are already stripped, and this was just a waste of time.