Page MenuHomeFreeBSD

libc: expose a primitive version of b64_pton()
Needs ReviewPublic

Authored by pstef on Jul 9 2022, 6:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 4:50 PM
Unknown Object (File)
Feb 8 2024, 7:22 PM
Unknown Object (File)
Jan 5 2024, 2:43 PM
Unknown Object (File)
Dec 20 2023, 6:21 AM
Unknown Object (File)
Dec 17 2023, 5:23 AM
Unknown Object (File)
Dec 12 2023, 12:46 PM
Unknown Object (File)
Nov 29 2023, 2:04 AM
Unknown Object (File)
Nov 22 2023, 5:34 PM
Subscribers

Details

Reviewers
kevans
kib
imp
Summary

b64_pton_partial() takes a pointer to pointer and will update it while decoding.
Then the caller will be able to copy the correctly decoded part, even if decoding another part yielded an error and aborted the process.

This change is very invasive. One bug I introduced during development of this patch made my ssh client dysfunctional.

Test Plan

If we want to go with this, I guess an exp-run would be a good idea.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

pstef requested review of this revision.Jul 9 2022, 6:10 PM
pstef created this revision.

Less buggy and visibly less invasive.

Fix the Symbols.map part of this diff.

pstef retitled this revision from Expose a primitive version of b64_pton() to libc: expose a primitive version of b64_pton().Jul 18 2022, 9:24 AM

Add the missing semicolon in Symbol.map.
Add base64.h to INCS.

Rename base64.h to b64.h.

include/b64.h
1

This file lacks:

  1. License preamble
  2. include guards
  3. DECLS guards

Also, other b64 symbols are in private namespace, while you are adding your symbol in the user namespace (note two underscores stuff)

7

Is this an interface we intend to support forever? From what I read so far in the patch, it is not pretty (to put it mildly) and in fact exposes some implementation details.

Why not to normalize the interface to make it more in line with usual C conventions? At least, stop updating pointer in place etc, look at memcpy as the exemplar design.

lib/libc/net/base64.c
44

This is wrong place for any include addition. There must be no any lines between cdefs.h and FBSDID (not least because both lines are going to be removed somewhere in the future)

323

return ();

@kib, sorry about not providing some aspects of the context of this change, I thought this was a good enough draft to get the first few reviews, but in hindsight it looks just lazy of me.

One part of the context is that I want to teach base64 to simply ignore garbage input, you can see that change proposed in D36293.

I'll address only some of the points you made because it looks like this patch is heading a new direction where some of my proposed changes are withdrawn.

Also, other b64 symbols are in private namespace, while you are adding your symbol in the user namespace (note two underscores stuff)

Yes, this was intentional and it's arguably an error of mine not to document this choice.
People use/abuse the private symbols, like in this example: https://www.lemoda.net/unix/base64/index.html so I wanted to expose a public and documented interface to do that (as of now, this patch fails to do the latter). This is also the reason I extracted the macro and the prototypes to a separate header file include/b64.h.

Is this an interface we intend to support forever? From what I read so far in the patch, it is not pretty (to put it mildly) and in fact exposes some implementation details.

I don't know what in practice it means to support something like this, hence my call for help.
I'm not sure what you find ugly about it, but I don't like the "goto error" and reduntant code parts; they are there because I tried to minimize the change to make the intent clear.
I could make b64_pton_partial() like mempcpy(), where you provide the source, the destination, the length and the function returns where it stopped writing (and it doesn't modify any object in place, except for the buffer whose modification is the point). This should give b64_pton() enough information to keep returninig -1 on wrong input and also it should enable me to teach base64 to ignore garbage.

Why not to normalize the interface to make it more in line with usual C conventions? At least, stop updating pointer in place etc, look at memcpy as the exemplar design.

I can see the similarity to mempcpy() but I fail to see how making b64_pton_partial() similar to memcpy() would enable the users to make use of the partial write in case of garbage input.