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.

Sign up for GitHub

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

Jump to bottom

fix: Links with unique targets should have unique labels #307

Merged
merged 4 commits into from
Jan 31, 2023
Merged

fix: Links with unique targets should have unique labels #307

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c262f15
fix: DLinks with unique targets should have unique labels
smockle Jan 25, 2023
1f30fbb
fix: Use 'written_footnote_ix' (index) in place of named tag
smockle Jan 25, 2023
4cc5541
fix: Use 'written_footnote_ix' (index) in place of named tag in subse…
smockle Jan 25, 2023
388ddec
feat: Output index in a data attribute
smockle Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/html.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,14 @@ static bool S_put_footnote_backref(cmark_html_renderer *renderer, cmark_strbuf *
if (renderer->written_footnote_ix >= renderer->footnote_ix)
return false;
renderer->written_footnote_ix = renderer->footnote_ix;
char m[32];
snprintf(m, sizeof(m), "%d", renderer->written_footnote_ix);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just as a note, my concern would be about snprintf implementations that don't nul terminate properly. If it were possible to get a 32byte width integer value out of the backref indexes (highly unlikely imo), we are not guaranteed a nul terminated string in m, which may lead to memory info leaks as cmark_strbuf_puts is strlen based (i.e. it will think len is 32 + whatever offset the first nul byte is located in adjacent memory). AFAICT everything that we build against in our test matrix is C99 conformant (but e.g. old visual studio versions pre-2015 were not iirc). /cc @kevinbackhouse for thoughts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we wanted to be abundantly careful re: C99 compliance we could maybe do something like:

Suggested change
snprintf(m, sizeof(m), "%d", renderer->written_footnote_ix);
char m[32];
memset(m, 0, sizeof(m));
snprintf(m, sizeof(m)-1, "%d", renderer->written_footnote_ix);

Copy link
Collaborator

Choose a reason for hiding this comment

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

As per @kevinbackhouse %d is not able to print an integer of that width, so this should be fine.

Copy link
Author

@smockle smockle Jan 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for raising this, @anticomputer! To clarify, by “this should be fine”, did you mean that the code in this PR is fine as-is, or that the snippet you provided above sufficiently addresses the issue you noted?

I’m happy to go with whatever you recommend. Should we change this elsewhere? I was following the examples below, and I don’t know C or this codebase well enough to understand if these are categorically different in some way:

cmark-gfm/src/html.c

Lines 74 to 75 in eb32891

char n[32];
snprintf(n, sizeof(n), "%d", i);

and

cmark-gfm/src/html.c

Lines 432 to 433 in eb32891

char n[32];
snprintf(n, sizeof(n), "%d", node->footnote.ref_ix);


cmark_strbuf_puts(html, "<a href=\"#fnref-");
houdini_escape_href(html, node->as.literal.data, node->as.literal.len);
cmark_strbuf_puts(html, "\" class=\"footnote-backref\" data-footnote-backref aria-label=\"Back to content\">↩</a>");
cmark_strbuf_puts(html, "\" class=\"footnote-backref\" data-footnote-backref aria-label=\"Back to reference ");
cmark_strbuf_puts(html, m);
cmark_strbuf_puts(html, "\">↩</a>");

if (node->footnote.def_count > 1)
{
Expand All @@ -78,7 +82,11 @@ static bool S_put_footnote_backref(cmark_html_renderer *renderer, cmark_strbuf *
houdini_escape_href(html, node->as.literal.data, node->as.literal.len);
cmark_strbuf_puts(html, "-");
cmark_strbuf_puts(html, n);
cmark_strbuf_puts(html, "\" class=\"footnote-backref\" data-footnote-backref aria-label=\"Back to content\">↩<sup class=\"footnote-ref\">");
cmark_strbuf_puts(html, "\" class=\"footnote-backref\" data-footnote-backref aria-label=\"Back to reference ");
cmark_strbuf_puts(html, m);
cmark_strbuf_puts(html, "-");
cmark_strbuf_puts(html, n);
cmark_strbuf_puts(html, "\">↩<sup class=\"footnote-ref\">");
cmark_strbuf_puts(html, n);
cmark_strbuf_puts(html, "</sup></a>");
}
Expand Down
12 changes: 6 additions & 6 deletions test/extensions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,23 +737,23 @@ Hi!
<section class="footnotes" data-footnotes>
<ol>
<li id="fn-1">
<p>Some <em>bolded</em> footnote definition. <a href="#fnref-1" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>Some <em>bolded</em> footnote definition. <a href="#fnref-1" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1">↩</a></p>
</li>
<li id="fn-footnote">
<blockquote>
<p>Blockquotes can be in a footnote.</p>
</blockquote>
<pre><code>as well as code blocks
</code></pre>
<p>or, naturally, simple paragraphs. <a href="#fnref-footnote" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>or, naturally, simple paragraphs. <a href="#fnref-footnote" class="footnote-backref" data-footnote-backref aria-label="Back to reference 2">↩</a></p>
</li>
<li id="fn-other-note">
<p>no code block here (spaces are stripped away) <a href="#fnref-other-note" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>no code block here (spaces are stripped away) <a href="#fnref-other-note" class="footnote-backref" data-footnote-backref aria-label="Back to reference 3">↩</a></p>
</li>
<li id="fn-codeblock-note">
<pre><code>this is now a code block (8 spaces indentation)
</code></pre>
<a href="#fnref-codeblock-note" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a>
<a href="#fnref-codeblock-note" class="footnote-backref" data-footnote-backref aria-label="Back to reference 4">↩</a>
</li>
</ol>
</section>
Expand All @@ -773,7 +773,7 @@ This footnote is referenced[^a-footnote] multiple times, in lots of different pl
<section class="footnotes" data-footnotes>
<ol>
<li id="fn-a-footnote">
<p>This footnote definition should have three backrefs. <a href="#fnref-a-footnote" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a> <a href="#fnref-a-footnote-2" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩<sup class="footnote-ref">2</sup></a> <a href="#fnref-a-footnote-3" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩<sup class="footnote-ref">3</sup></a></p>
<p>This footnote definition should have three backrefs. <a href="#fnref-a-footnote" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1">↩</a> <a href="#fnref-a-footnote-2" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1-2">↩<sup class="footnote-ref">2</sup></a> <a href="#fnref-a-footnote-3" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1-3">↩<sup class="footnote-ref">3</sup></a></p>
</li>
</ol>
</section>
Expand All @@ -790,7 +790,7 @@ Hello[^"><script>alert(1)</script>]
<section class="footnotes" data-footnotes>
<ol>
<li id="fn-%22%3E%3Cscript%3Ealert(1)%3C/script%3E">
<p>pwned <a href="#fnref-%22%3E%3Cscript%3Ealert(1)%3C/script%3E" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>pwned <a href="#fnref-%22%3E%3Cscript%3Ealert(1)%3C/script%3E" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1">↩</a></p>
</li>
</ol>
</section>
Expand Down
14 changes: 7 additions & 7 deletions test/regression.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ A footnote in a paragraph[^1]
<section class="footnotes" data-footnotes>
<ol>
<li id="fn-1">
<p>a footnote <a href="#fnref-1" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a> <a href="#fnref-1-2" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩<sup class="footnote-ref">2</sup></a></p>
<p>a footnote <a href="#fnref-1" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1">↩</a> <a href="#fnref-1-2" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1-2">↩<sup class="footnote-ref">2</sup></a></p>
</li>
</ol>
</section>
Expand Down Expand Up @@ -284,10 +284,10 @@ This is some text. It has a citation.[^citation]
<section class="footnotes" data-footnotes>
<ol>
<li id="fn-citation">
<p>This is a long winded parapgraph that also has another citation.<sup class="footnote-ref"><a href="#fn-another-citation" id="fnref-another-citation" data-footnote-ref>2</a></sup> <a href="#fnref-citation" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>This is a long winded parapgraph that also has another citation.<sup class="footnote-ref"><a href="#fn-another-citation" id="fnref-another-citation" data-footnote-ref>2</a></sup> <a href="#fnref-citation" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1">↩</a></p>
</li>
<li id="fn-another-citation">
<p>My second citation. <a href="#fnref-another-citation" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>My second citation. <a href="#fnref-another-citation" class="footnote-backref" data-footnote-backref aria-label="Back to reference 2">↩</a></p>
</li>
</ol>
</section>
Expand All @@ -306,10 +306,10 @@ This is some text. It has two footnotes references, side-by-side without any spa
<section class="footnotes" data-footnotes>
<ol>
<li id="fn-footnote1">
<p>Hello. <a href="#fnref-footnote1" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>Hello. <a href="#fnref-footnote1" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1">↩</a></p>
</li>
<li id="fn-footnote2">
<p>Goodbye. <a href="#fnref-footnote2" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>Goodbye. <a href="#fnref-footnote2" class="footnote-backref" data-footnote-backref aria-label="Back to reference 2">↩</a></p>
</li>
</ol>
</section>
Expand All @@ -331,10 +331,10 @@ It has another footnote that contains many different characters (the autolinker
<section class="footnotes" data-footnotes>
<ol>
<li id="fn-widely-cited">
<p>this renders properly. <a href="#fnref-widely-cited" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>this renders properly. <a href="#fnref-widely-cited" class="footnote-backref" data-footnote-backref aria-label="Back to reference 1">↩</a></p>
</li>
<li id="fn-sphinx-of-black-quartz_judge-my-vow-0123456789">
<p>so does this. <a href="#fnref-sphinx-of-black-quartz_judge-my-vow-0123456789" class="footnote-backref" data-footnote-backref aria-label="Back to content">↩</a></p>
<p>so does this. <a href="#fnref-sphinx-of-black-quartz_judge-my-vow-0123456789" class="footnote-backref" data-footnote-backref aria-label="Back to reference 2">↩</a></p>
</li>
</ol>
</section>
Expand Down