Add @base64d for decoding base64 #47

[l8nite]

This adds an @base64d to do base64 decoding (issue #47).

I needed this as part of querying Consul's HTTP API, which returns k/v values base64 encoded in the result JSON.

Edit: The appveyor failures appear unrelated to my PR (something about maximum file size on the download of Oniguruma).

[nicowilliams]

If the input isn't base64, shouldn't we terminate early and return an error (jv_invalid_with_msg())? Particularly as we're reading the string a byte at a time rather than a Unicode codepoint at a time.

[l8nite]

Yea there are a couple places where we should bail out if something goes amiss, I will make the code more defensive.

[nicowilliams]

This is cool, thanks. See review comments (so far only one). Also, maybe add a test with invalid base64 data? It should produce an errors (use try ... catch ... to check that you get an error).

[nicowilliams]

The Appveyor build had what I hope is a transient failure due to it not being able to find Oniguruma. I sure hope it's transient. Anyways, you can ask Appveyor to rebuild: visit their site, click sign-in, click sign-in with github credentials, go to your projects page and find jq there, then click on the current build, click on rebuild.

[nicowilliams]

VLAs are dangerous. Suppose the incoming string is 10MB... and suppose the stack grows down... you could easily start by writing to memory that's not on the stack but in some other memory mapping. In the best case you'd just segfault. I would much prefer this be malloc()ed.

[nicowilliams]

appveyor/ci#613

[nicowilliams]

Last comment for a while: long-term I want to have support for binary strings represented as arrays of small integers (0..255), but internally as plain unsigned char arrays, naturally. So what I'd like to do with base64-encoded binary data then is to decode into this binary data "type" (it wouldn't be a proper type; type would report it as "array"). We might need a new text format specifier for decoding base64-encoded data into this "type".

[l8nite]

@nicowilliams Thanks for the review. I've switched to alloca/memset for the result array, and I added error conditions for invalid base64 data and the case where there are not enough bytes in the input to successfully decode a character.

I added a new base64 test suite independent of the others so it'd be a little faster to run, let me know if you want me to squash those back into jq.test instead.

Edit: Appveyor appears to still be failing, I'll keep an eye on your related ticket.

[nicowilliams]

@l8nite alloca() has exactly the same problem as VLAs (the two are morally the same thing: change the sp by the requested number of bytes and good luck to you).

You could do this (which I've done elsewhere):

/*
 * Make alloca() safe by tripping any guard page if there is one and we
 * want to allocate enough bytes to blow the stack.
 */
static void
ensure_alloca(size_t bytes)
{
    int stack_grows_down;
    size_t i;
    volatile char *mem = alloca(bytes);

    stack_grows_down = &mem[bytes - 1] < &mem[0];
    for (i = 0; i < bytes; i += 4096) {
        if (stack_grows_down)
            ((char *)mem)[bytes - i] = 0;
        else
            ((char *)mem)[i] = 0;
    }
}

#define SAFE_ALLOCA(bytes) (ensure_alloca(bytes), alloca(bytes))

which will merely segfault if you request too much memory, assuming there's a guard page anyways, as opposed to possibly corrupting memory. (And this assumes pages are 4KB, which they might not be). But still, that's a terrible way to fail.

Just malloc()/free() :)

[l8nite]

@nicowilliams Thanks! You're exposing my seriously-underutilized C skills here ;) Change pushed.

[nicowilliams]

Yes, you have to jv_free(input); before returning.

[l8nite]

Turns out jv_free was already being called by type_error so all good there. I've squashed the commits down into the initial commit. Let me know if there's anything else I can fix.

[nicowilliams]

@l8nite For now there's nothing to do. I can't get AppVeyor to build. I've tried several things, and I'll try more tomorrow. Without clean builds, we won't merge.

[nicowilliams]

I found the maintainer of the Oniguruma pkgs for MSYS. I hope he responds.

[nicowilliams]

If we don't hear from him I might just adapt https://github.com/Alexpux/MINGW-packages/tree/master/mingw-w64-oniguruma to build in appveyor.yml.

[l8nite]

I've tried a couple builds this week on my own appveyor account without success. Any word from the Onigurama maintainer?

[nicowilliams]

No word yet. I'll have to find time for making appveyor.yml build oniguruma :(

[shanielh]

Hi mate, I was trying to do the same, Instead I was using base64 command (I'm using linux), I've created import/export script for the KV store of consul :

map("curl \"http://NEW_ADDR:8500/kv/\(.Key)\" -XPUT -d \"$(echo \"\(.Value)\" | base64 --decode)\"")[]

Good luck with the pull-request.

[royalaid]

Just curious as to the status of this

[nicowilliams]

Sorry, i've been traveling.

What I'll do is disable the appveyor build in appveyor.yml and pull this. Probably tonight.

[dansteen]

+1 for this! It will make working with consul so much easier.

[so0k]

Any update on this? Please advise any help needed to get this merged?

[wtlangford]

I'll take a look at this tonight, and if it's good, I'll rebase and merge it.

…

On Sun, Feb 12, 2017, 04:04 Vincent De Smet ***@***.***> wrote: Any update on this? Please advise any help needed to get this merged? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1089 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADQ4VxYJg9lO1EVx2S5_sMXtbA046qD1ks5rbssbgaJpZM4HLQ_A> .

[wtlangford]

Rebased and merged manually with e6fa039

[nicowilliams]

Thanks @wtlangford for reviewing and merging! Thanks @l8nite for the contribution!