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

Augeas add lenspath to errormessage #3286

Merged
merged 2 commits into from Nov 28, 2019
Merged

Augeas add lenspath to errormessage #3286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e747336
augeas: added lenspath to error message
Nov 27, 2019
1ffee26
news: added release notes
Nov 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/news/_preparation_next_release.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ You can also read the news [on our website](https://www.libelektra.org/news/0.9.

The following section lists news about the [modules](https://www.libelektra.org/plugins/readme) we updated in this release.

### Augeas

- Improved error message for augeas to show lensPath. _(Michael Zronek)_

### <<Plugin1>>

- <<TODO>>
Expand Down
15 changes: 10 additions & 5 deletions src/plugins/augeas/augeas.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,17 @@ int elektraAugeasGenConf (KeySet * ks, Key * errorKey ELEKTRA_UNUSED)
return retval;
}

static const char * getAugeasError (augeas * augeasHandle)
static const char * getAugeasError (augeas * augeasHandle, const char * lensPath)
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 you can also make the pointer const, if you want to.

Suggested change
static const char * getAugeasError (augeas * augeasHandle, const char * lensPath)
static const char * getAugeasError (augeas * augeasHandle, const char * const lensPath)

{
const char * reason = 0;
if (aug_error (augeasHandle) != 0)
{
const char * format = "%s\n\tlensPath: %s";
reason = aug_error_message (augeasHandle);
size_t messageSize = strlen (reason) + strlen (lensPath) + strlen (format);
Copy link
Member

Choose a reason for hiding this comment

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

As far as I know strlen only returns the number of characters of a string.

The strlen() function returns the number of characters that precede the terminating NUL character.

I think you need an additional byte for the terminating \0 character, otherwise buffer will not be able to hold the entire error message.

Suggested change
size_t messageSize = strlen (reason) + strlen (lensPath) + strlen (format);
size_t messageSize = strlen (reason) + strlen (lensPath) + strlen (format) + 1;

Copy link
Author

Choose a reason for hiding this comment

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

I have copied this from here:

libelektra/src/plugins/augeas/augeas.c

Lines 143 to 150 in e747336

const char * format = "%s\n\tposition: %s:%s\n\tmessage: %s\n\tlens: %s";
size_t messageSize = (augeasError ? strlen (augeasError) : 0) + (line ? strlen (line) : 0) +
(character ? strlen (character) : 0) + (message ? strlen (message) : 0) +
(lens ? strlen (lens) : 0) + strlen (format);
char * buffer = elektraMalloc (messageSize);
snprintf (buffer, messageSize, format, augeasError ? augeasError : "", line ? line : "", character ? character : "",
message ? message : "", lens ? lens : "");
reason = buffer;

This bug should then also appear there

Copy link
Member

Choose a reason for hiding this comment

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

This bug should then also appear there

I think you are right. The off-by-one error is also one of the reasons why elektraStrLen also counts the terminating NULL character.

libelektra/src/libs/elektra/internal.c

Lines 369 to 371 in f3460ca

* This function differs from strlen() because it is Unicode and multibyte
* chars safe. While strlen() counts characters and ignores the final NULL,
* elektraStrLen() count bytes including the ending NULL.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kodebach can you take a look here?

Copy link
Member

Choose a reason for hiding this comment

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

What exactly should I look at?

@sanssecours is right in general, but actually the + 1 is not needed here. The string format is a printf-string in the resulting buffer the %s will not be present, but they are counted by strlen. So actually the allocated buffer is 3 bytes bigger than it needs to be.

I didn't check the second one. It might be more problematic, because in the NULL cases, strlen is replaced by 0, but the NULL string is replaced by " ". I think it still works, but adding 1 to the buffer size certainly doesn't hurt.

PS If someone is working on augeas, there are code paths where it sets an error, but continues to execute and in the end returns SUCCESS.

Copy link
Member

Choose a reason for hiding this comment

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

The string format is a printf-string in the resulting buffer the %s will not be present, but they are counted by strlen.

I did not think about that 😊. Thank you for the detailed explanation.

char * buffer = elektraMalloc (messageSize);
snprintf (buffer, messageSize, format, reason, lensPath);
reason = buffer;
}
else
{
Expand Down Expand Up @@ -408,7 +413,7 @@ static int saveTree (augeas * augeasHandle, KeySet * ks, const char * lensPath,
if (ret < 0)
{
/* report the augeas specific error */
ELEKTRA_SET_VALIDATION_SYNTACTIC_ERROR (parentKey, getAugeasError (augeasHandle));
ELEKTRA_SET_VALIDATION_SYNTACTIC_ERROR (parentKey, getAugeasError (augeasHandle, lensPath));
}

return ret;
Expand Down Expand Up @@ -504,7 +509,7 @@ int elektraAugeasGet (Plugin * handle, KeySet * returned, Key * parentKey)
if (ret < 0)
{
fclose (fh);
ELEKTRA_SET_INSTALLATION_ERROR (parentKey, getAugeasError (augeasHandle));
ELEKTRA_SET_INSTALLATION_ERROR (parentKey, getAugeasError (augeasHandle, lensPath));
errno = errnosave;
return -1;
}
Expand Down Expand Up @@ -536,7 +541,7 @@ int elektraAugeasGet (Plugin * handle, KeySet * returned, Key * parentKey)
{
fclose (fh);
ksDel (append);
ELEKTRA_SET_INSTALLATION_ERROR (parentKey, getAugeasError (augeasHandle));
ELEKTRA_SET_INSTALLATION_ERROR (parentKey, getAugeasError (augeasHandle, lensPath));
errno = errnosave;
return -1;
}
Expand Down Expand Up @@ -590,7 +595,7 @@ int elektraAugeasSet (Plugin * handle, KeySet * returned, Key * parentKey)
if (ret < 0)
{
fclose (fh);
ELEKTRA_SET_INSTALLATION_ERROR (parentKey, getAugeasError (augeasHandle));
ELEKTRA_SET_INSTALLATION_ERROR (parentKey, getAugeasError (augeasHandle, lensPath));
errno = errnosave;
return -1;
}
Expand Down