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

Fix ASAN errors due adding offset to nullptr #240

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

parmeet
Copy link

@parmeet parmeet commented Jun 8, 2022

This PR fixes the error when Address Sanitization is enabled. The offset is added to nullptr which gives following runtime error:

applying non-zero offset 4 to null pointer

The PR checks whether the pointer is nullptr, and if so do not add offset to it.

utf8proc.c Outdated
Comment on lines 368 to 377
if (!dst){
written += utf8proc_decompose_char(entry_cp, dst,
(bufsize > written) ? (bufsize - written) : 0, options,
last_boundclass);
}else{
written += utf8proc_decompose_char(entry_cp, dst+written,
(bufsize > written) ? (bufsize - written) : 0, options,
last_boundclass);
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe cleaner to combine into a single call with a dst ? ... argument

Suggested change
if (!dst){
written += utf8proc_decompose_char(entry_cp, dst,
(bufsize > written) ? (bufsize - written) : 0, options,
last_boundclass);
}else{
written += utf8proc_decompose_char(entry_cp, dst+written,
(bufsize > written) ? (bufsize - written) : 0, options,
last_boundclass);
}
written += utf8proc_decompose_char(entry_cp,
dst ? dst+written : dst,
(bufsize > written) ? (bufsize - written) : 0, options,
last_boundclass);

@stevengj
Copy link
Member

stevengj commented Jun 9, 2022

(Note that the ASAN warning is harmless in this case, since utf8proc_decompose_char doesn't write anything if bufsize is 0 as will be the case here. But we might as well silence ASAN here.)

@parmeet
Copy link
Author

parmeet commented Jun 9, 2022

(Note that the ASAN warning is harmless in this case, since utf8proc_decompose_char doesn't write anything if bufsize is 0 as will be the case here. But we might as well silence ASAN here.)

Actually it's not just about the warning. I have noticed runtime error caused due to this that makes the program crash.

@stevengj
Copy link
Member

I have noticed runtime error caused due to this that makes the program crash.

Could you elaborate? I don't see any if (dst == NULL) checks or similar, so I don't see what preserving NULL could accomplish (other than silencing ASAN).

@parmeet
Copy link
Author

parmeet commented Jun 14, 2022

@stevengj This is my rough understanding what's going on:

When utf8proc_decompose_custom is called here

result = utf8proc_decompose_custom(str, strlen, NULL, 0, options, custom_func, custom_data);
inside utf8proc_map_custom with buffer == NULL, and wpos is incremented inside the while loop, it is adding wpos (>0) to buffer which is NULL. This give run-time error applying non-zero offset 4 to null pointer. By incorporating changes in this PR, I could get rid of these run-time errors.

@stevengj
Copy link
Member

That sounds like a runtime error caused by ASAN. Applying a nonzero offset to a null pointer is perfectly safe if you never dereference the pointer.

@xaizek
Copy link

xaizek commented Feb 21, 2024

That sounds like a runtime error caused by ASAN. Applying a nonzero offset to a null pointer is perfectly safe if you never dereference the pointer.

Answers to this question suggest that it's a UB. Although I'm not sure how come NULL pointer is "a pointer into, or just beyond, an array object", but clang generates an undefined behaviour error for this which suggest it might be a valid interpretation.

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.

3 participants