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

book.md: fix bib reference typo for array bounds post #228

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

kees
Copy link
Contributor

@kees kees commented Feb 4, 2024

Removes extra []s around the @Cook2023 bib reference so the URL will render correctly.


This change is Reviewable

book.md Outdated
@@ -1332,7 +1332,7 @@ will include bounds checks.
Successfully using bounds checking compiler features for a large codebase
requires substantial effort. An example of this is refactoring the Linux kernel
to use bounds checks for flexible arrays, as described in [Kees Cook's
blog]([@Cook2023]).
blog](@Cook2023).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
blog](@Cook2023).
blog](https://people.kernel.org/kees/bounded-flexible-arrays-in-c).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reporting this @kees !
When I tested locally, just removing the square brackets still resulted in a broken link.
Ultimately, I think the root issue here is mixing citation syntax (starting with @ syntax) and the markdown syntax to create a regular link.
For this reference, I thought that probably it would be best to have a regular link, as the target is a blog post, rather than a formal citation. Not going with a citation also means that the reader gets taken to your blog post directly rather than having an indirection through the bibliography and having to click a second time to get to the blog post.

Does that seem allright to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just trying to follow the examples in the rest of the doc. For example, the bib linkage for Aleph's stuff seems to work as expected. It seems without the linkage, the array-bounds blog post doesn't end up appearing at the end of the document either, so I think we should fix it to show up. I noticed others have comments like "leave this with a trailing comma" but I didn't understand what that was about. Is that needed here for some reason?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where those "leave this with a trailing comma" comments come from in the book.bib file. Maybe @g-kouv knows? (Although don't expect an answer soon, I expect Georgia to be offline for a while).

I'm happy either way with having an entry in the bibliography or linking directly. Thanks for fixing this!
I'll be merging this soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I experimented with this: a trailing comma means "do not order by surname". So I have no trailing comma and correctly appear as "Cook, Kees" but Aleph One does, and correctly appears as "Aleph One" in the references.

@kbeyls
Copy link
Member

kbeyls commented Feb 5, 2024

Oh - one more question: this project follows all-contributors, see https://github.com/llsoftsec/llsoftsecbook?tab=readme-ov-file#contributors-
Would you be OK to be added there?

@kees
Copy link
Contributor Author

kees commented Feb 5, 2024

Oh - one more question: this project follows all-contributors, see https://github.com/llsoftsec/llsoftsecbook?tab=readme-ov-file#contributors- Would you be OK to be added there?

Ah, cool! Yes, that would be fine. :)

Remove URL wrapper for Cook2023 bib reference. Additionally mark "C" as
a separate span so it remains capitalized.
@kees
Copy link
Contributor Author

kees commented Feb 5, 2024

Okay, I've actually build tested this now. (I didn't have docker installed, but now I've got it all working.) This appears to be a proper fix now. (Commit has been updated.)

@kbeyls
Copy link
Member

kbeyls commented Feb 5, 2024

@all-contributors please add @kees for code

Copy link
Contributor

@kbeyls

I've put up a pull request to add @kees! 🎉

@kbeyls kbeyls merged commit b742df0 into llsoftsec:main Feb 5, 2024
2 checks passed
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

Successfully merging this pull request may close these issues.

2 participants