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

net/sock_util: Accept NULL pointers in urlsplit #11677

Merged
merged 2 commits into from
Jun 12, 2019
Merged

net/sock_util: Accept NULL pointers in urlsplit #11677

merged 2 commits into from
Jun 12, 2019

Conversation

leandrolanzieri
Copy link
Contributor

Contribution description

This PR modifies the implementation of sock_urlsplit so it can accept NULL pointers for either the address and/or the path part. This way it's more flexible to use, as one does not have to allocate a buffer for both parts if one of them is not needed.

There are also two new unit tests for testing this conditions.

Testing procedure

Run the unit tests:

cd tests/unittests
make tests-sock_util test

Issues/PRs references

None

@leandrolanzieri leandrolanzieri added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Jun 11, 2019
jcarrano
jcarrano previously requested changes Jun 11, 2019
View reviewed changes
Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

Just a couple of details. Otherwise it is OK.

sys/net/sock/sock_util.c
@@ -116,30 +117,35 @@ static char* _find_pathstart(const char *url)

int sock_urlsplit(const char *url, char *hostport, char *urlpath)
{
assert(url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert(url);
assert(url != NULL);

Copy link
Contributor

Choose a reason for hiding this comment

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

please keep the original authors style where it doesn't go against coding conventions.

sys/net/sock/sock_util.c Outdated
return -EOVERFLOW;
}
memcpy(hostport, hoststart, hostlen);
*(hostport + hostlen) = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*(hostport + hostlen) = '\0';
hostport[hostlen] = '\0';

Note: i know you didn't write that, but considering there are not many chances to do little fixes like this...

sys/net/sock/sock_util.c
* terminator */
if (hostlen > SOCK_HOSTPORT_MAXLEN - 1) {
return -EOVERFLOW;
if (hostport) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (hostport) {
if (hostport != NULL) {

sys/net/sock/sock_util.c
if (pathlen) {
if (pathlen > SOCK_URLPATH_MAXLEN - 1) {
return -EOVERFLOW;
if (urlpath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (urlpath) {
if (urlpath != NULL) {

sys/net/sock/sock_util.c Outdated
return -EOVERFLOW;
if (urlpath) {
size_t pathlen = strlen(pathstart);
if (pathlen) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This outer "if" is not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I think removing the if (pathlen) { statement introduces possible undefined behaviour with the memcpy() (in the case pathlen equals 0) below, unless you can show that pathlen here is always greater than 0.

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 can't see the issue here, isn't it valid to pass 0 length to memcpy?

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I think I confused myself here. IIRC it is undefined to call it with NULL as one of the arguments, even when the passed length is 0, but that's not the case here.

sys/net/sock/sock_util.c Outdated
}
memcpy(urlpath, pathstart, pathlen);
*(urlpath + pathlen) = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*(urlpath + pathlen) = '\0';
urlpath[pathlen] = '\0';

same as above, since you are you are touching the code, why not fix this too.

@smlng smlng added this to the Release 2019.07 milestone Jun 12, 2019
smlng
smlng suggested changes Jun 12, 2019
View reviewed changes
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

otherwise tested pre-ACK, works!

sys/net/sock/sock_util.c
@@ -116,30 +117,33 @@ static char* _find_pathstart(const char *url)

int sock_urlsplit(const char *url, char *hostport, char *urlpath)
{
assert(url != NULL);
char *hoststart = _find_hoststart(url);
if (!hoststart) {
Copy link
Member

Choose a reason for hiding this comment

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

as you changed the others this should be if (hoststart == NULL) { accordingly, or change back as suggested by @kaspar030 - but be consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed back to original style

sys/net/sock/sock_util.c Outdated
@@ -116,30 +117,33 @@ static char* _find_pathstart(const char *url)

int sock_urlsplit(const char *url, char *hostport, char *urlpath)
{
assert(url != NULL);
Copy link
Member

Choose a reason for hiding this comment

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

this addition should be reflected in the documentation as follows:

@param[in]   url         URL to split. Not Null.
[...]
@pre `url != NULL`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@smlng smlng self-assigned this Jun 12, 2019
miri64
miri64 reviewed Jun 12, 2019
View reviewed changes
sys/include/net/sock/util.h Outdated Show resolved Hide resolved
smlng
smlng approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

re-tested and ACK again. Please squash and amend wording suggested by @miri64.

@smlng
Copy link
Member

smlng commented Jun 12, 2019

@jcarrano please have a look again

@smlng smlng added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jun 12, 2019
@leandrolanzieri leandrolanzieri force-pushed the pr/net/sock_util_split_null branch from 6609ab9 to 864247d Compare June 12, 2019 08:04
@jcarrano jcarrano dismissed their stale review June 12, 2019 09:42

Requests addressed.

@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 12, 2019
@smlng
Copy link
Member

smlng commented Jun 12, 2019

ack and go

@smlng smlng merged commit d5bdb5d into RIOT-OS:master Jun 12, 2019
@leandrolanzieri
Copy link
Contributor Author

Thanks for reviewing!

@leandrolanzieri leandrolanzieri deleted the pr/net/sock_util_split_null branch June 12, 2019 11:21
@MrKevinWeiss MrKevinWeiss added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bergzand bergzand bergzand left review comments

@miri64 miri64 miri64 left review comments

@smlng smlng smlng approved these changes

@jcarrano jcarrano jcarrano left review comments

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees

@smlng smlng

Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2019.07
Development

Successfully merging this pull request may close these issues.
7 participants
@leandrolanzieri @smlng @miri64 @jcarrano @kaspar030 @bergzand @MrKevinWeiss